Skip to content

Merge OKD-specific changes into master#2778

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
vrutkovs:master-okd
Jan 28, 2022
Merged

Merge OKD-specific changes into master#2778
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
vrutkovs:master-okd

Conversation

@LorbusChris
Copy link
Copy Markdown
Contributor

I'm opening this draft PR to open a forum for MCO and OKD maintainers to discuss how the OKD-specific changes to the MCO code base can be merged back into master. This would allow for dropping the fork and lighten the OKD maintenance burden.

I'd like to ask @fortinj66 and @vrutkovs to expand on the reasoning behind those individual changes, and provide links to associated issue/bug reports (if any). I am open to opening more fine-grained PRs against master for each issue addressed with these changes.

My hopes are this will make it easier for @openshift/openshift-team-mco to review the changes.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Sep 28, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot requested review from jstuever and yboaron September 28, 2021 12:59
@LorbusChris
Copy link
Copy Markdown
Contributor Author

/test okd-e2e-aws

@LorbusChris
Copy link
Copy Markdown
Contributor Author

/test e2e-aws

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2021
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2021
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2021
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021
@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

I know this is a draft, but just noting for future discussion, wondering why this is reverting a (relatively) new crio PR: #2805

I think for this type of change would prefer to do it as a separate PR, but that can wait for now while this is still in draft.

@vrutkovs
Copy link
Copy Markdown
Contributor

vrutkovs commented Nov 12, 2021

I know this is a draft, but just noting for future discussion, wondering why this is reverting a (relatively) new crio PR: #2805

temporary fix until we get crio 1.23 released, slack thread (I wasn't aware this PR gets autoupdated)

Comment thread pkg/daemon/update.go Outdated
if err := validateExtensions(newConfig.Spec.Extensions); err != nil && dn.os.IsRHCOS() {
return err
}
// if err := validateExtensions(newConfig.Spec.Extensions); err != nil && dn.os.IsRHCOS() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably can be dropped - afaik we don't use extensions since 4.8, so it won't affect upgrades.
I'll give that a try later on

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would be a no-op on FCOS anyway, due to the err != nil && dn.os.IsRHCOS(), no?

Comment thread pkg/daemon/update.go Outdated
// from /var/home/core/.ssh/authorized_keys.d/ignition if missing
if err := dn.writeMissingAuthorizedKeys(); err != nil {
return err
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrutkovs this commit can be dropped in the next rebase now that #2688 has merged

Comment thread pkg/daemon/kernelargs.go Outdated
return tuneableFCOSArgsAllowlist[arg], nil
}
return false, nil
return true, nil
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	} else if os.IsFCOS() {
		return true, nil
	}
	return false, nil

in order to still consider other systems this may run on, like RHEL.

Comment thread cmd/machine-config-daemon/pivot.go Outdated
}

// Check to see if we need to tune kernel arguments
tuningChanged, err := daemon.UpdateTuningArgs(daemon.KernelTuningFile, daemon.CmdLineFile)
Copy link
Copy Markdown
Contributor Author

@LorbusChris LorbusChris Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're not using the kernel-args file anymore for the bootstrap pivot in 4.9+, I think this commit can be dropped too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use kernel-args for bootstrap

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now that openshift/installer#5439 has merged, the installer doesn't use it anymore either. I think that was the last place?

Restart=always
ExecStartPre=-/bin/sh -c '/usr/bin/chown -R ${OVS_USER_ID} /var/lib/openvswitch'
ExecStartPre=-/bin/sh -c '/usr/bin/chown -R ${OVS_USER_ID} /etc/openvswitch'
ExecStartPre=-/bin/sh -c '/usr/bin/chown -R ${OVS_USER_ID} /run/openvswitch'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to check if upgrades from 4.9 to 4.10 are still affected by this. If not, this commit can be dropped.

@jstuever
Copy link
Copy Markdown
Contributor

jstuever commented Dec 9, 2021

/uncc

@openshift-ci openshift-ci Bot removed the request for review from jstuever December 9, 2021 20:15
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2021
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2021
@vrutkovs vrutkovs force-pushed the master-okd branch 2 times, most recently from 37613d3 to f302aa1 Compare December 18, 2021 08:38
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 21, 2021

@LorbusChris: 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-gcp-op f302aa1 link true /test e2e-gcp-op

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.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jan 14, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LorbusChris
To complete the pull request process, please assign yuqi-zhang after the PR has been reviewed.
You can assign the PR to them by writing /assign @yuqi-zhang in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@vrutkovs vrutkovs force-pushed the master-okd branch 2 times, most recently from 90c99ca to adcc3bb Compare January 23, 2022 09:23
Recent podman in F35 is hanging during `cat`
@openshift-merge-robot openshift-merge-robot merged commit 1307ccf into openshift:master Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants