-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-8447: MCO-496: Support ignition versions 3.3 + 3.4 but keep version 3.2 as default #3576
OCPBUGS-8447: MCO-496: Support ignition versions 3.3 + 3.4 but keep version 3.2 as default #3576
Conversation
@jkyros: This pull request references MCO-496 which is a valid jira issue. 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. |
Skipping CI for Draft Pull Request. |
/test e2e-gcp-op |
1 similar comment
/test e2e-gcp-op |
/test e2e-aws-ovn-upgrade |
9994593
to
ce4cda5
Compare
@jkyros: This pull request references MCO-496 which is a valid jira issue. 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. |
785406a
to
c40c4f6
Compare
@jkyros: This pull request references MCO-496 which is a valid jira issue. 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. |
/test all |
0563818
to
206c01e
Compare
// can get down to 3.2 | ||
ignCfgV3_3, errV3_3 := v34tov33.Translate(ignCfgV3) | ||
if errV3_3 != nil { | ||
// This case should only be hit if fields that only exist in v3_4 are being used which are not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Would an example of those fields be the tang / kernel args fields?
If so, I don't think that would produce an error if the MCO can't support it; only if the Ignition payload cannot be translated from one Ignition version to another. We may need a check that those unsupported fields aren't being used so we can explicitly show an error vs. silently failing. If those already exist, then please disregard my question and concern 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, correct. The "tang/kernel args fields" are exactly what I'm referring to.
The checks you're describing do exist but not in the MCO -- they're in ign-converter, e.g;
and they're applied as part of the "down-translation" process, so I'm "freeloading" on that so we don't have to duplicate that bit of code into the MCO.
And so yes, if you feed us an ignition containing offline tang fields:
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
labels:
machineconfiguration.openshift.io/role: worker
name: 99-cant-convert-34
spec:
config:
ignition:
version: 3.4.0
storage:
luks:
- name: luks-tang
device: "/dev/sdb"
clevis:
tang:
- url: https://tang.example.com
thumbprint: REPLACE-THIS-WITH-YOUR-TANG-THUMBPRINT
advertisement: '{"payload": "...", "protected": "...", "signature": "..."}'
Much like any other render/conversion error, your pool will degrade:
NAME CONFIG UPDATED UPDATING DEGRADED MACHINECOUNT READYMACHINECOUNT UPDATEDMACHINECOUNT DEGRADEDMACHINECOUNT AGE
master rendered-master-a79af855fcc5ad725c369aa16a2d6f32 True False False 3 3 3 0 46m
worker rendered-worker-8664798e8b1c6588299c00a261886237 True False True 3 3 3 0 46m
Last Transition Time: 2023-03-01T20:36:33Z
Message: Failed to render configuration for pool worker: translating Ignition config failed with error Invalid input config: tang offline provisioning is not supported in spec v3.3
Reason:
Status: True
Type: RenderDegraded
And the render controller will whine about it:
I0301 20:36:38.767708 1 render_controller.go:377] Error syncing machineconfigpool worker: translating Ignition config failed with error Invalid input config: tang offline provisioning is not supported in spec v3.3
For the record I'd absolutely love to have an admission controller to parse and reject it up front. Maybe someday 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we bump our default from 3.2 to 3.4 without the MCO implementing the features described in 3.3 and 3.4 then yes we should do exactly what you describe.
206c01e
to
928616a
Compare
Revendored now that coreos/ign-converter#44 merged |
For the record, this is actually a new practice, driven by the requirement for clean downgrade tests.
The general design here is that 4.13 will support any configs declaring themselves as 3.4.0 that are also translatable to 3.2.0, right? And then in 4.14, we can start enabling 3.4.0 features, which is fine because we only need to support downgrades to 4.13 when no 4.14-specific features are configured. That seems okay in general, but if MCO 4.14 starts translating MachineConfig
Offline Tang shouldn't need explicit MCO support AFAIK; the field just needs to be passed through to Ignition. However, I noticed that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, thanks for working on this! Some questions below
// ConvertRawExtIgnitionToV3_4 ensures that the Ignition config in | ||
// the RawExtension is spec v3.4, or translates to it. | ||
func ConvertRawExtIgnitionToV3_4(inRawExtIgn *runtime.RawExtension) (runtime.RawExtension, error) { | ||
// TODO(jkyros): since 3.4 is "ahead" of our current default 3.2, we're going "up" not down, which is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think this comment was for the V3_3 conversion (Once we bump to 3.3 this will be briefly obsolete
should be 3.4, and the rest will be future proofing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You correct sir, good catch 😄
ignCfgV3_2, rptV3_2, errV3_2 := ign3.Parse(rawIgn) | ||
if errV3_2 == nil && !rptV3_2.IsFatal() { | ||
// ParseCompatibleVersion will parse any config <= N to version N | ||
ignCfgV3, rptV3, errV3 := ign3_4.ParseCompatibleVersion(rawIgn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice! We no longer have to chain ourselves 👍
} | ||
serveConf = &converted34 | ||
|
||
} else if reqConfigVer.Equal(*semver.New("3.3.0")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think RHCOS has been on 3.3 for awhile, and technically we've been feeding it 3.2 up to now. I don't think this would change any actual functionality though right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that and wasn't sure if things cared if they got a different version than they asked for.
It shouldn't change any actual functionality, but we can totally keep feeding them 3.2
if you're worried.
That probably turns into "If they ask for a 3.x version > our default, give them our default" .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine to send a newer config version. Ignition uptranslates older configs internally anyway.
I believe this is https://bugzilla.redhat.com/show_bug.cgi?id=1964701 + https://issues.redhat.com/browse/RFE-1894 ? |
IMO those issues are a bit different, in that they cover one case where the MCO might want to learn to reconcile |
We were kind of "too chicken" to fix it by just degrading because the folks in those issues Jerry linked were fixing it by updating it in MachineConfig (for any new nodes that come online), and then retrofitting their old ones via SSH or whatever. If we degrade, they'd have to retrofit out-of-band forever, whereas if we let them change it they can say "from this point on, all the new nodes will be right". |
Right. I was more so pointing to the fact that some users may be depending on one behaviour already. If we want to switch to "degrade" or "MCO manages it" instead of "the silent treatment", I think we would need to consider the ramifications to switch. I would be tentatively be ok with us degrading, if only to warn the user changes may be coming in future versions |
/test e2e-gcp-op |
Verified using IPI on GCP with proxy. Steps that we followed in order to verity the PR (similar to those in #3576 (comment)):
The behavior was the expected one.The result was the same that we got in the first verification.
The behavior was the expected one. The result was the same that we got in the first verification.
The behavior was the expected one. The result was the same that we got in the first verification. Note that the kernel arguments are not present in the 3.4.0/3.3.0 versions of the ignition configs.
The MCP was not degraded. Since this comment this issue is expected #3576 (comment) We can think about creating a different jira issue to track this problem.
The behavior was the expected one. The result was the same that we got in the first verification.
The behavior was the expected one. The result was the same that we got in the first verification.
All files were created properly. The error in ignition version 2.2.0 does not happen anymore.
Both 2.2.0 and 3.4.0 version behaved as expected. New rendered 3.2.0 machineconfigs were created and the units worked as expected
No issues found
One issue found. We have a test (TC OCP-42390) to make sure that when the ignition version is empty, the MCP is degraded showing this message:
Instead of this message now we get this other message
We can modify the expected test message, but we would like to make sure that the message that we use is the one that we actually want to use. I think that the TC's expected message is more descriptive and helpful than the one we are reporting now. Apart from that minor issue, no failure was found and everything is working as expected. Once we decide if we want to modify this error message or keep it, we will add the qe-approved label. Thank you very much for your great job!! |
This adds MCS api support so the MCS can serve ignition 3.3 if requested, but the MCO will still default to ignition 3.2 internally. In order to keep 3.2 as the default/internal version while supporting the newer versions, this parses incoming configs up to 3.3 and then uses the ignition converter to "down-translate" them back to version 3.2.
This adds MCS api support so the MCS can serve ignition 3.4 if requested, but the MCO will still default to ignition 3.2 internally. In order to keep 3.2 as the default/internal version while supporting the newer versions, this parses incoming configs up to 3.4 and then uses the ignition converter to "down-translate" them back to version 3.2 (via 3.3, so 3.4->3.3->3.2). This does not include MCO implementations of the new features added by the new ignition spec, just support for the spec itself.
A future MCO is going to understand and implement ignition kargs, but this one doesn't, and we need to allow for that This: - adds the ignition kargs ShouldExist into MachineConfig kargs if ignition kargs are present - scrapes the ignition kargs out of our 3.3+ ignition configs during down-translation so we can still down-translate - degrades if ShouldNotExist is present in ignition kargs, since we don't have a way to handle it in this MCO version This code will all come out when the MCO actually implements ignition kargs, it's just a "bridge" between versions so we can downgrade without breaking. .
GOPROXY=direct go get github.com/coreos/ign-converter/translate/v33tov32 GOPROXY=direct go get github.com/coreos/ign-converter/translate/v34tov33 go get github.com/coreos/ignition/v2/config/v3_4 make go-deps
997e78d
to
c251e55
Compare
Rebased and added a more helpful error message for "Invalid Version" . @sergiordlr I kind of "split the difference" on the error message. The new ignition parsing introduced in this PR differentiates between The new error message for the
(I just handled that error type explicitly as a separate case, and changed unknown to invalid, so we could tell by the logs which case we hit) |
The error reported now is similar to the one reported before this PR.
We add the qe-approved label. @jkyros Thank you very much!! /label qe-approved |
/test e2e-gcp-op |
1 similar comment
/test e2e-gcp-op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, nice work John!
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkyros, sinnykumari, 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 |
Thanks for looking at it Sinny, and since we have QE approval, it looks like we're good to go |
What a weird e2e-aws-ovn failure. That's been green the whole time. I'm struggling to correlate anything we've done here to service recreation failing.
/test e2e-aws-ovn |
@jkyros: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
Looks like we hit a grace period on that one. Feels like global issues. We'll try one more time.
/test e2e-aws-ovn |
@jkyros: Jira Issue OCPBUGS-8447: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-8447 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. |
I figured this was going to have to be a manual backport, but the manual backport seems to think it's clean so we'll try the cherry-pick bot |
@jkyros: new pull request created: #3714 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. |
Stabilise openshift 4.14.0 spec on fcos 1.5 and ignition 3.4 stable specs. The MCO is now capable of understanding Ignition 3.4.0 specs and uses that by default. See: openshift/machine-config-operator#3576 See: openshift/machine-config-operator#3814
This allows the MCO to receive and serve ignition versions 3.3 and 3.4, but DOES NOT change the default from 3.2, and as discussed does not include MCO feature implementations for the kernel args feature nor the "offline tang" feature (lest we introduce more risk).
The MCO has traditionally tried to support an ignition version at least one release before it's actually used so we have the ability to roll back an upgrade, etc. This PR is in service to that goal so we can use 3.4 in the next version, and next version we will come back and do the "rest of the work" to move the default version to 3.4.
- What I did
Added ignition 3.3 version support (but not support for the kargs feature yet)
Added ignition 3.4 version support (but not support for the offline tang feature yet)
Updated unit tests to account for the new versions
Bumped ignition deps for ignition 3.4
This means that:
- How to verify it
Feed the MCO something in the ignition 3.3/3.4 format:
Feed the MCO something in the ignition 3.3/3.4 format that contains fields not supported in 3.2 (kargs, offline tang):
Request a 3.3/3.4 version ignition from the MCS:
- Description for the changelog
Allow MCO to serve and receive ignition spec versions 3.3 and 3.4