OCPBUGS-1482: Don't override schedulableMasters unnecessarily #4414

zaneb · 2022-09-19T20:38:31Z

When generating the Ignition files, the installer already sets schdulableMasters to true when there are no worker nodes (i.e. in the SNO and compact cluster topologies. (See
openshift/installer#2004) Therefore it is unnecessary to override it here (though it may be preferred to avoid a warning log from the installer).

Since openshift/installer#6247, attempting to override the schedulableMasters setting causes installation to fail, because there are two manifests of the same type and name that conflict.

Since we don't need to set this override when the installer would already do it, avoid doing so and triggering the error when the value is determined by the number of hosts rather than explicitly set by the user.

The conflict still needs to be resolved so that the user can enable schedulableMasters, but this at least allows the SNO and compact topologies to install OpenShift 4.12 again.

This partially reverts commit c45f369.

List all the issues related to this PR

https://issues.redhat.com/browse/OCPBUGS-1482

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
[] None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

When generating the Ignition files, the installer already sets schdulableMasters to true when there are no worker nodes (i.e. in the SNO and compact cluster topologies. (See openshift/installer#2004) Therefore it is unnecessary to override it here (though it may be preferred to avoid a warning log from the installer). Since openshift/installer#6247, attempting to override the schedulableMasters setting causes installation to fail, because there are two manifests of the same type and name that conflict. Since we don't need to set this override when the installer would already do it, avoid doing so and triggering the error when the value is determined by the number of hosts rather than explicitly set by the user. The conflict still needs to be resolved so that the user can enable schedulableMasters, but this at least allows the SNO and compact topologies to install OpenShift 4.12 again. This partially reverts commit c45f369.

openshift-ci-robot · 2022-09-19T20:38:34Z

@zaneb: This pull request references Jira Issue OCPBUGS-1482, which is invalid:

expected the bug to target the "rhacm-2.6" version, but it targets "4.12.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

When generating the Ignition files, the installer already sets schdulableMasters to true when there are no worker nodes (i.e. in the SNO and compact cluster topologies. (See
openshift/installer#2004) Therefore it is unnecessary to override it here (though it may be preferred to avoid a warning log from the installer).

Since openshift/installer#6247, attempting to override the schedulableMasters setting causes installation to fail, because there are two manifests of the same type and name that conflict.

Since we don't need to set this override when the installer would already do it, avoid doing so and triggering the error when the value is determined by the number of hosts rather than explicitly set by the user.

The conflict still needs to be resolved so that the user can enable schedulableMasters, but this at least allows the SNO and compact topologies to install OpenShift 4.12 again.

This partially reverts commit c45f369.

List all the issues related to this PR

https://issues.redhat.com/browse/OCPBUGS-1482

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

[] None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

zaneb · 2022-09-19T20:39:53Z

/test e2e-agent-compact

codecov · 2022-09-19T20:54:13Z

Codecov Report

Merging #4414 (46731bf) into master (ef1a928) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4414      +/-   ##
==========================================
- Coverage   66.71%   66.71%   -0.01%     
==========================================
  Files         195      196       +1     
  Lines       27335    27355      +20     
==========================================
+ Hits        18236    18249      +13     
- Misses       7468     7475       +7     
  Partials     1631     1631

Impacted Files	Coverage Δ
internal/common/common.go	`32.08% <100.00%> (ø)`
...rollers/hypershiftagentserviceconfig_controller.go	`65.00% <0.00%> (ø)`

openshift-ci · 2022-09-19T23:08:31Z

@zaneb: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agent-compact	`46731bf`	link	false	`/test e2e-agent-compact`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

osherdp · 2022-09-20T07:28:23Z

/uncc
/cc @adriengentil

carbonin · 2022-09-20T18:11:08Z

When generating the Ignition files, the installer already sets schdulableMasters to true when there are no worker nodes (i.e. in the SNO and compact cluster topologies

Is this the case for all versions we support?
Are we going to end up not setting schedulable masters for compact clusters for older versions that we still support like 4.8?

carbonin · 2022-09-20T18:24:13Z

/test ?

openshift-ci · 2022-09-20T18:24:16Z

@carbonin: The following commands are available to trigger required jobs:

/test edge-ci-index
/test edge-e2e-ai-operator-ztp
/test edge-e2e-ai-operator-ztp-converged
/test edge-e2e-ai-operator-ztp-sno-day2-workers
/test edge-e2e-ai-operator-ztp-sno-day2-workers-late-binding
/test edge-e2e-metal-assisted
/test edge-e2e-metal-assisted-4-10
/test edge-e2e-metal-assisted-4-11
/test edge-e2e-metal-assisted-4-12
/test edge-e2e-metal-assisted-4-8
/test edge-e2e-metal-assisted-4-9
/test edge-images
/test edge-lint
/test edge-subsystem-aws
/test edge-subsystem-kubeapi-aws
/test edge-unit-test
/test edge-verify-generated-code
/test images
/test mce-images

The following commands are available to trigger optional jobs:

/test e2e-agent-compact
/test edge-e2e-ai-operator-ztp-3masters
/test edge-e2e-ai-operator-ztp-capi
/test edge-e2e-ai-operator-ztp-compact-day2-workers
/test edge-e2e-ai-operator-ztp-disconnected
/test edge-e2e-ai-operator-ztp-ipv4v6-3masters-ocp-410
/test edge-e2e-ai-operator-ztp-ipv4v6-3masters-ocp-49
/test edge-e2e-ai-operator-ztp-ipv4v6-sno-ocp-410
/test edge-e2e-ai-operator-ztp-ipv4v6-sno-ocp-49
/test edge-e2e-ai-operator-ztp-multiarch-3masters-ocp-411
/test edge-e2e-ai-operator-ztp-multiarch-sno-ocp-411
/test edge-e2e-ai-operator-ztp-sno-day2-workers-ignitionoverride
/test edge-e2e-metal-assisted-capi
/test edge-e2e-metal-assisted-cnv
/test edge-e2e-metal-assisted-day2
/test edge-e2e-metal-assisted-day2-single-node
/test edge-e2e-metal-assisted-ipv4v6
/test edge-e2e-metal-assisted-ipv6
/test edge-e2e-metal-assisted-kube-api-late-binding-single-node
/test edge-e2e-metal-assisted-kube-api-late-unbinding-ipv4-single-node
/test edge-e2e-metal-assisted-kube-api-net-suite
/test edge-e2e-metal-assisted-none
/test edge-e2e-metal-assisted-ocs
/test edge-e2e-metal-assisted-odf
/test edge-e2e-metal-assisted-onprem
/test edge-e2e-metal-assisted-single-node
/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-tang
/test edge-e2e-metal-assisted-tpmv2
/test edge-e2e-vsphere-assisted
/test edge-e2e-vsphere-assisted-umn
/test edge-push-pr-image
/test push-pr-image

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-assisted-service-master-edge-ci-index
pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-ztp
pull-ci-openshift-assisted-service-master-edge-e2e-metal-assisted
pull-ci-openshift-assisted-service-master-edge-images
pull-ci-openshift-assisted-service-master-edge-lint
pull-ci-openshift-assisted-service-master-edge-subsystem-aws
pull-ci-openshift-assisted-service-master-edge-subsystem-kubeapi-aws
pull-ci-openshift-assisted-service-master-edge-unit-test
pull-ci-openshift-assisted-service-master-edge-verify-generated-code
pull-ci-openshift-assisted-service-master-images
pull-ci-openshift-assisted-service-master-mce-images

Details

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

carbonin · 2022-09-20T19:21:45Z

Are we going to end up not setting schedulable masters for compact clusters for older versions that we still support like 4.8?

Looks like the change to the installer is in 4.8 as openshift/installer@64f96df

openshift-ci · 2022-09-20T19:23:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [carbonin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

carbonin · 2022-09-20T19:23:49Z

Let's make sure we handle this properly in a follow up.
Whatever "properly" ends up being.

That said it doesn't seem like this will cause a problem and it will unblock the agent team so I'm good with merging.

carbonin · 2022-09-20T19:26:37Z

This is indeed referencing a bug, and it's an issue regarding the agent installer targeted to 4.12.

IMO this shouldn't be an invalid bug.
I'll manually remove the label and hope it doesn't get added back.

openshift-ci-robot · 2022-09-20T21:22:23Z

/retest-required

Remaining retests: 0 against base HEAD 7312ee7 and 2 for PR HEAD 46731bf in total

openshift-ci-robot · 2022-09-20T22:51:55Z

…openshift#4414)" This reverts commit 5661d9c.

filanov · 2022-09-28T06:13:08Z

/cherry-pick release-ocm-2.6

openshift-cherrypick-robot · 2022-09-28T06:13:56Z

@filanov: new pull request created: #4445

Details

In response to this:

/cherry-pick release-ocm-2.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…openshift#4414)" This reverts commit 5661d9c. Let's go back to applying the schedulable masters patch when either SchedulableMastersForce or SchedulableMasters are set.

* MGMT-12435: Add a way to apply patches to core manifests Starting in 4.12 it's not possible to completely overwrite existing manifests, the way AI was doing it. Core manifests that need to be modified should be patched instead. This PR adds a logic to upload patches to the manifests api and then apply them. Note that the InfrastructureCR patching was not changed to us this new flow as this PR focuses only in adding this possibility and changing the SchedulableMasters flow. Future PRs will align the rest of the codebase. Signed-off-by: Flavio Percoco <[email protected]> * Defer delete of manifests dir in case of error Signed-off-by: Flavio Percoco <[email protected]> * Address review comments Signed-off-by: Flavio Percoco <[email protected]> * Revert "OCPBUGS-1482: Don't override schedulableMasters unnecessarily (#4414)" This reverts commit 5661d9c. Let's go back to applying the schedulable masters patch when either SchedulableMastersForce or SchedulableMasters are set. Signed-off-by: Flavio Percoco <[email protected]> Co-authored-by: Flavio Percoco <[email protected]>

…openshift#4414)" This reverts commit 5661d9c. Let's go back to applying the schedulable masters patch when either SchedulableMastersForce or SchedulableMasters are set.

…e manifests (#4636) * MGMT-12435: Add a way to apply patches to core manifests Starting in 4.12 it's not possible to completely overwrite existing manifests, the way AI was doing it. Core manifests that need to be modified should be patched instead. This PR adds a logic to upload patches to the manifests api and then apply them. Note that the InfrastructureCR patching was not changed to us this new flow as this PR focuses only in adding this possibility and changing the SchedulableMasters flow. Future PRs will align the rest of the codebase. Signed-off-by: Flavio Percoco <[email protected]> * Defer delete of manifests dir in case of error Signed-off-by: Flavio Percoco <[email protected]> * Address review comments Signed-off-by: Flavio Percoco <[email protected]> * Revert "OCPBUGS-1482: Don't override schedulableMasters unnecessarily (#4414)" This reverts commit 5661d9c. Let's go back to applying the schedulable masters patch when either SchedulableMastersForce or SchedulableMasters are set. Signed-off-by: Flavio Percoco <[email protected]> Co-authored-by: Flavio Percoco <[email protected]>

…openshift#4414)" This reverts commit 5661d9c. Let's go back to applying the schedulable masters patch when either SchedulableMastersForce or SchedulableMasters are set.

…able masters (#4653) * MGMT-12435: Add a way to apply patches to core manifests Starting in 4.12 it's not possible to completely overwrite existing manifests, the way AI was doing it. Core manifests that need to be modified should be patched instead. This PR adds a logic to upload patches to the manifests api and then apply them. Note that the InfrastructureCR patching was not changed to us this new flow as this PR focuses only in adding this possibility and changing the SchedulableMasters flow. Future PRs will align the rest of the codebase. Signed-off-by: Flavio Percoco <[email protected]> * Defer delete of manifests dir in case of error Signed-off-by: Flavio Percoco <[email protected]> * Address review comments Signed-off-by: Flavio Percoco <[email protected]> * Revert "OCPBUGS-1482: Don't override schedulableMasters unnecessarily (#4414)" This reverts commit 5661d9c. Let's go back to applying the schedulable masters patch when either SchedulableMastersForce or SchedulableMasters are set. Signed-off-by: Flavio Percoco <[email protected]> Co-authored-by: Flavio Percoco <[email protected]>

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 19, 2022

openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 19, 2022

openshift-ci bot requested review from danielerez and osherdp September 19, 2022 20:39

openshift-ci bot requested review from adriengentil and removed request for osherdp September 20, 2022 07:28

carbonin approved these changes Sep 20, 2022

View reviewed changes

openshift-ci bot assigned carbonin Sep 20, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2022

carbonin removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 20, 2022

openshift-merge-robot merged commit 5661d9c into openshift:master Sep 20, 2022

wking added a commit to wking/assisted-service that referenced this pull request Sep 21, 2022

Revert "OCPBUGS-1482: Don't override schedulableMasters unnecessarily (…

e94f5b0

…openshift#4414)" This reverts commit 5661d9c.

openshift-cherrypick-robot mentioned this pull request Sep 28, 2022

[release-ocm-2.6] OCPBUGS-1482: Don't override schedulableMasters unnecessarily #4445

Merged

OCPBUGS-1482: Don't override schedulableMasters unnecessarily #4414

OCPBUGS-1482: Don't override schedulableMasters unnecessarily #4414

Uh oh!

Conversation

zaneb commented Sep 19, 2022

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Sep 19, 2022

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

zaneb commented Sep 19, 2022

Uh oh!

codecov bot commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

openshift-ci bot commented Sep 19, 2022

Uh oh!

osherdp commented Sep 20, 2022

Uh oh!

carbonin commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carbonin commented Sep 20, 2022

Uh oh!

openshift-ci bot commented Sep 20, 2022

Uh oh!

carbonin commented Sep 20, 2022

Uh oh!

openshift-ci bot commented Sep 20, 2022

Uh oh!

carbonin commented Sep 20, 2022

Uh oh!

carbonin commented Sep 20, 2022

Uh oh!

openshift-ci-robot commented Sep 20, 2022

Uh oh!

openshift-ci-robot commented Sep 20, 2022

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

filanov commented Sep 28, 2022

Uh oh!

openshift-cherrypick-robot commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Sep 19, 2022 •

edited

Loading

carbonin commented Sep 20, 2022 •

edited

Loading