pkg/asset/ignition/bootstrap/cvoignore: Add group/kind/name(space) collision detection#6247
Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom Sep 14, 2022
Conversation
…llision detection docs/design/resource_dep.svg shows the complicated hierarchy of installer assets, and it is likely that not all users are familiar with all the manifests that the installer generates. It's also possible that there is a complicated hierarchy on the caller's side, and not just a single actor, creating additional manifests to feed into a create-manifests flow. With all of these inputs, it seems possible that folks occasionally call the installer with directory contents that result in multiple manifests for a single resource, as determined by the (group, kind, namespace, name) tuples. This commit adds a check at the slightly-before-Ignition-serialization overrides asset, which was already in the business of iterating over the content and building ClusterVersion spec.overrides. Failing fast before we build Ignition configs should allow for relatively cheap recovery, vs. a user's cluster failing to come up, or coming up with a config that diverges from their intention because some other manifest ended up taking precedence over the resource that they'd been trying to control. Not covered in this commit is deduping between manifests generated on the bootstrap machine (e.g. by the various 'render' calls to operator containers). But that can happen orthogonally, or not, and I think it's still worth having this cheap, pre-Ignition-config sanity check.
Member
Author
|
Lots of /retest |
Contributor
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
/lgtm
/assign @jhixson74
/retest
| namespace := u.GetNamespace() | ||
| name := u.GetName() | ||
|
|
||
| key := fmt.Sprintf("%s |! %s |! %s |! %s", group, kind, namespace, name) |
Contributor
There was a problem hiding this comment.
Out of curiosity, why the |! separator?
Member
Author
There was a problem hiding this comment.
I didn't spend much time poking around in allowed-characters for the four entries, and I wanted a delimiter that was unlikely to show up in any of them.
Member
Author
There was a problem hiding this comment.
it would perhaps be more robust to just use %q%q%q%q?
Member
|
This looks straight forward to me, I just want to see the tests pass first ;-) |
Member
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhixson74 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
/skip |
Contributor
Contributor
Contributor
|
/skip |
Contributor
Contributor
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Contributor
|
/hold Revision 1c9e68d was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
Member
Author
|
/retest |
openshift-ci
Bot
removed
the
do-not-merge/hold
wking
added a commit
to wking/openshift-release
that referenced
this pull request
Sep 15, 2022
…nflict Try to confirm openshift/installer#6247 works as expected
Member
Author
|
Confirmed that this works as expected, with an error message that could be polished further, but which is already good enough to satisfy me ;) |
zaneb
added a commit
to zaneb/assisted-service
that referenced
this pull request
Sep 19, 2022
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.
Merged
19 tasks
openshift-merge-robot
pushed a commit
to openshift/assisted-service
that referenced
this pull request
Sep 20, 2022
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-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/assisted-service
that referenced
this pull request
Sep 28, 2022
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-merge-robot
pushed a commit
to openshift/assisted-service
that referenced
this pull request
Sep 29, 2022
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. Co-authored-by: Zane Bitter <zbitter@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
docs/design/resource_dep.svgshows the complicated hierarchy of installer assets, and it is likely that not all users are familiar with all the manifests that the installer generates. It's also possible that there is a complicated hierarchy on the caller's side, and not just a single actor, creating additional manifests to feed into a create-manifests flow. With all of these inputs, it seems possible that folks occasionally call the installer with directory contents that result in multiple manifests for a single resource, as determined by the (group, kind, namespace, name) tuples. This commit adds a check at the slightly-before-Ignition-serialization overrides asset, which was already in the business of iterating over the content and building ClusterVersion spec.overrides. Failing fast before we build Ignition configs should allow for relatively cheap recovery, vs. a user's cluster failing to come up, or coming up with a config that diverges from their intention because some other manifest ended up taking precedence over the resource that they'd been trying to control.Not covered in this commit is deduping between manifests generated on the bootstrap machine (e.g. by the various 'render' calls to operator containers). But that can happen orthogonally, or not, and I think it's still worth having this cheap, pre-Ignition-config sanity check.