Bug 1881703: Revert https://github.com/openshift/machine-config-operator/pull/1792 #2126
Conversation
|
@runcom: An error was encountered adding this pull request to the external tracker bugs for bug 1881703 on the Bugzilla server at https://bugzilla.redhat.com:
In response to this:
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-ci-robot
added
the
approved
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/severity-high
|
@runcom: This pull request references Bugzilla bug 1881703, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
There was a problem hiding this comment.
I think given the timelines I'm +1 for the revert. Since this isn't an exact one-to-one I just want to make sure the diffs are good:
- We did not seem to have removed the
KubeMAOSharedInformerin controller_context.go - We did not revert
renderAssetfunc typing in render.go - We didn't remove one line from go.sum
- We didn't remove variable definitions (this is fine)
Those are the diffs I saw
Right, we do need to support that. It is actually one of the only sane ways to have per machine data (see also #1720 ). |
|
Are you proposing to revert https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md fully for 4.6? The more I look at this BZ the more I feel like being able to hand-edit the Ignition in a machineAPI managed setup was somewhat of an accidental feature. As I commented in the BZ I think they should be using MachineConfig which would transparently just work. (Actually at a higher level I think they could be taking better advantage of all the networking enhancements in 4.6 but it looks like they're using baremetal IPI which doesn't use those yet) |
yes, while the BZ can be "fixed" by shipping an MC I'm more worried about the change in behavior that we've effectively introduced with an half-baked feature like that - we should really take into account the translation from 4.5 or just v2 and also we don't really know if users are using this but since it's kind of advertised it's a safe assumption. The thing is there could be clusters where we would just overwrite any pointer ignition config customization done at installation so I think it's safe to revert and perfection the implementation in a future release, does it sound good? |
|
+1 on removing and pushing a redo later |
|
There's definitely risk to trying to back this out at the last minute - if the issue can be addressed by having the user provide MCs, it also seems reasonable to me to mark this as a known issue and move on. |
@cgwalters just to clarify: you're saying leave it as-is and say "the supported way to do this is via a machine-config" if we receive a bz? But later fix or not ? |
right, there's definitely some risk at reverting as well after ~1 month that this is soaking into master... I think I'd be open writing something in bold that says "if you were doing this, use an MC from now on when installing a cluster" - but then, what do we do about already installed clusters? we're still screwing up their scale-up but I think we can also document a way out of that (if there's one?)? |
Hmmm. That is indeed more of a potential problem. The BZ reporter was talking about new installs, but we also clearly want to support them upgrading in-place to 4.6. So far we only have evidence that 4.5 baremetal users are customizing the pointer config, but not for other platforms. I strongly suspect the percentage of cloud/IaaS users (azure/gcp/aws/etc) doing this is effectively zero.
Honestly I'm mostly arguing for a bit more consideration/debate, I just want to be sure we have explored the alternatives and weighed the drawbacks a bit more. To clarify: if after this the rest of the team is still +1 on reverting, that's fine by me! |
|
So if we do the revert...it means that the MCO won't manage the worker userdata, which shouldn't break anything in new 4.6 installs or upgrades, all that we have lost is future support for updating bootimages per
from the enhancement. Right? |
oh yeah, I think I didn't mean to push the revert (and sorry if I did) - I opened the reverts to have something immediately actionable given where we are time-wise in the release - let me put an hold on this meanwhile we discuss /hold |
openshift-ci-robot
added
the
do-not-merge/hold
yes, when doing the bootimages upgrade feature, we'd need to re-take this into account |
|
OK I'm leaning towards revert indeed, let's just be "on point" to make sure that everything works afterwards. On this specific PR - we could also just comment out the code doing the secret sync right? That'd make it less of a conflict-fest to reintroduce later in 4.7. But I'm fine as is too. |
|
I'm still +1 on the revert here too. Just to make sure:
|
|
cross linking openshift/machine-api-operator#715 and openshift/installer#4228 machine-api is merged, I will approve installer, and we can lgtm this after we're sure no regressions happen |
Signed-off-by: Antonio Murdaca <runcom@linux.com>
removed
this shouldn't affect anything, we use the type as interface and it made more sense to type the func with that - reverted tho
I've run
should be ok to go! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: runcom, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@runcom: All pull requests linked via external trackers have merged: Bugzilla bug 1881703 has been moved to the MODIFIED state. In response to this:
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. |
This PR just reverts Manage the ignition stub config #1792
More information can be found at https://docs.google.com/document/d/1TnnfPgFim-e895MD0msOGN9tyPBk3CM8cmSewFmgzDQ/edit#
TL;DR; users can customize the pointer ignition config adding files and whatever ignition supports. We weren't aware of that and assumed only the MCS endpoint and the cert would be managed by the installer - given where we are and the assessment that it's safe to just revert, let's do it and re-think the work in light of the new findings (as it'll require a translation as well..)
Signed-off-by: Antonio Murdaca runcom@linux.com