Bug 1890074: pkg/daemon: avoid duplicate extension verification by vrutkovs · Pull Request #2178 · openshift/machine-config-operator

vrutkovs · 2020-10-24T09:33:02Z

When package aliases (or groups) were introduced recently,
this list is now being verified twice - during validateExtensions on
MCC and in generateExtensionsArgs on MCD. This breaks OKD installs,
as we'd like to avoid verifying a pre-approved list of RPMs on FCOS.

However, MCC can't know which systems it would manage, so it applies
RHCOS allowlist on all systems unknowingly.

This commit refactors extensions verification and application:

available packages are listed in constants.SupportedPackages
groups/aliases MCD knows about are listed in constants.SupportedPackageAliases
validateExtensions ensures that spec.extensions items are in either list
generateExtensionsArgs expands aliases into a list of RPMs

On OKD validateExtensions is not called, however generateExtensionsArgs
would still apply (so kernel-devel would automatically bring in kernel-headers).
This might bite us in the future, but at least it won't block
installs, so its safe to merge now.

/cc @cgwalters @sinnykumari @kikisdeliveryservice

openshift-ci-robot · 2020-10-24T09:33:06Z

@vrutkovs: This pull request references Bugzilla bug 1890074, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

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

Details

In response to this:

Bug 1890074: pkg/daemon: avoid duplicate extension verification

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 · 2020-10-24T09:33:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrutkovs
To complete the pull request process, please assign ashcrow after the PR has been reviewed.
You can assign the PR to them by writing /assign @ashcrow 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:

OWNERS

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

openshift-ci-robot · 2020-10-24T09:36:56Z

@vrutkovs: This pull request references Bugzilla bug 1890074, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

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

Details

In response to this:

Bug 1890074: pkg/daemon: avoid duplicate extension verification

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.

When package aliases (or groups) were introduced recently, this list is now being verified twice - during `validateExtensions` on MCC and in `generateExtensionsArgs` on MCD. This breaks OKD installs, as we'd like to avoid verifying a pre-approved list of RPMs on FCOS. However, MCC can't know which systems it would manage, so it applies RHCOS allowlist on all systems unknowingly. This commit refactors extensions verification and application: * available packages are listed in `constants.SupportedPackages` * groups/aliases MCD knows about at listed in `constants.SupportedPackageAliases` * `validateExtensions` ensures that `spec.extensions` items are in either list * `generateExtensionsArgs` expands aliases into a list of RPMs On OKD `validateExtensions` is not called, however `generateExtensionsArgs` would still apply. This might bite us later, but at least it won't block install, so its safe to merge now

openshift-ci-robot · 2020-10-24T10:13:03Z

@vrutkovs: This pull request references Bugzilla bug 1890074, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

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

Details

In response to this:

Bug 1890074: pkg/daemon: avoid duplicate extension verification

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 · 2020-10-24T10:13:50Z

@vrutkovs: This pull request references Bugzilla bug 1890074, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

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

Details

In response to this:

Bug 1890074: pkg/daemon: avoid duplicate extension verification

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.

vrutkovs · 2020-10-24T13:36:46Z

/retest

rhel7/gcp-op flakes

vrutkovs · 2020-10-24T18:31:39Z

/test e2e-gcp-op

openshift-merge-robot · 2020-10-24T20:42:23Z

@vrutkovs: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-op	`2c171cd`	link	`/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.

sinnykumari · 2020-10-26T09:42:08Z

Thank you for creating the PR to fix the OKD issue.
I have created an alternate PR to fix this issue #2181 . Once OKD base package stabilizes, it encourages to group all the packages as a single extension which OKD can have it installed by default.

vrutkovs · 2020-10-26T09:47:10Z

Once OKD base package stabilizes, it encourages to group all the packages as a single extension which OKD can have it installed by default

I don't think we want that. Some packages may be shipped in OS image, but some may be installed from Fedora repos, so we'd prefer to avoid restricting OKD users to a hardcoded list of images

sinnykumari · 2020-10-26T10:17:58Z

Once OKD base package stabilizes, it encourages to group all the packages as a single extension which OKD can have it installed by default

I don't think we want that. Some packages may be shipped in OS image, but some may be installed from Fedora repos, so we'd prefer to avoid restricting OKD users to a hardcoded list of images

Hmm, right. Using Fedora repo is another possibility for users to layer packages.
Just curious, won't this make hard to keep track of what user is installing and issues OKD cluster may get during upgrade?

#2181 preserves the previous implementation of extension on FCOS, so OKD existing package layering approach should stay as it is.

vrutkovs · 2020-10-26T10:23:56Z

Just curious, won't this make hard to keep track of what user is installing and issues OKD cluster may get during upgrade?

It certainly would, that would be user responsibility though. In order to boot its sufficient to avoid removing default list of packages added by installer.

#2181 would work, we might want to investigate unrestricted package list later.

/hold

sinnykumari · 2020-10-26T10:36:22Z

#2181 would work, we might want to investigate unrestricted package list later.

definitely!

vrutkovs · 2020-10-28T16:37:30Z

Closed in favor of #2181

openshift-ci-robot requested review from cgwalters, kikisdeliveryservice and sinnykumari October 24, 2020 09:33

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Oct 24, 2020

vrutkovs mentioned this pull request Oct 24, 2020

Add initial machine-os-content for OKD 4.7 openshift/okd-machine-os#1

Merged

2 tasks

vrutkovs force-pushed the extension-aliases branch from 5fe81c7 to 666528d Compare October 24, 2020 10:10

vrutkovs force-pushed the extension-aliases branch from 666528d to 2c171cd Compare October 24, 2020 10:11

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2020

vrutkovs closed this Oct 28, 2020

Conversation

vrutkovs commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 24, 2020

Uh oh!

openshift-ci-robot commented Oct 24, 2020

Uh oh!

openshift-ci-robot commented Oct 24, 2020

Uh oh!

openshift-ci-robot commented Oct 24, 2020

Uh oh!

openshift-ci-robot commented Oct 24, 2020

Uh oh!

vrutkovs commented Oct 24, 2020

Uh oh!

vrutkovs commented Oct 24, 2020

Uh oh!

openshift-merge-robot commented Oct 24, 2020

Uh oh!

sinnykumari commented Oct 26, 2020

Uh oh!

vrutkovs commented Oct 26, 2020

Uh oh!

sinnykumari commented Oct 26, 2020

Uh oh!

vrutkovs commented Oct 26, 2020

Uh oh!

sinnykumari commented Oct 26, 2020

Uh oh!

vrutkovs commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vrutkovs commented Oct 24, 2020 •

edited

Loading