Skip to content
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

e2e: introduce upgrade suite #1073

Merged
merged 2 commits into from
Dec 24, 2024

Conversation

rbaturov
Copy link
Contributor

Starting in version 4.18, NROP MachineConfigs containing the custom SELinux policy are expected to be removed unless a specific annotation is set in the NUMAResourcesOperator CR to enforce the use of the custom (legacy) SELinux policy. To ensure this behavior, we added a test that verifies MachineConfigs are removed when the annotation is absent in the CR.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2024
Copy link
Contributor

openshift-ci bot commented Nov 14, 2024

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

@rbaturov
Copy link
Contributor Author

/test all

@rbaturov rbaturov changed the title e2e: test machineconfigs are removed custom policy enabled e2e: verify NROP machineconfigs are removed Nov 14, 2024
@rbaturov rbaturov marked this pull request as ready for review November 17, 2024 08:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2024
@rbaturov
Copy link
Contributor Author

/retest-required

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024
@rbaturov rbaturov force-pushed the post-upgrade-test branch 2 times, most recently from 8da6754 to e4fcfb7 Compare November 25, 2024 14:13
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024
@rbaturov rbaturov force-pushed the post-upgrade-test branch 2 times, most recently from 1f194f9 to 04d1879 Compare December 4, 2024 12:37
@rbaturov
Copy link
Contributor Author

rbaturov commented Dec 4, 2024

@ffromani @Tal-or @shajmakh
Ready for another review iteration

hack/run-test-upgrade-e2e.sh Outdated Show resolved Hide resolved
test/e2e/upgrade/upgrade_test.go Show resolved Hide resolved
@ffromani
Copy link
Member

ffromani commented Dec 4, 2024

what I'm missing is how this suite is meant to be used: preconditions, expectations...

scratch that, just need to re-read the description better

@rbaturov
Copy link
Contributor Author

rbaturov commented Dec 5, 2024

/retest-required

@ffromani
Copy link
Member

ffromani commented Dec 5, 2024

/retest-required

won't pass till #1099 merges, or till the real root issue gets fixed (which we have no visibility about)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2024
@rbaturov rbaturov force-pushed the post-upgrade-test branch 2 times, most recently from c477d81 to 8215648 Compare December 8, 2024 14:52
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2024
@ffromani
Copy link
Member

ffromani commented Dec 11, 2024

once #1111 lands, we can reimplement the version check using the info bundled in the operator container image, avoiding any extra deps.
To find the operator pod we can add something

$ cat test/utils/deploy/find.go 
/*
 * Copyright 2024 Red Hat, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package deploy

import (
	"context"
        "errors"
	"fmt"

	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/klog/v2"

	"sigs.k8s.io/controller-runtime/pkg/client"

	nropv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1"
)

func FindNUMAResourcesOperatorPod(ctx context.Context, cli client.Client, nrop *nropv1.NUMAResourcesOperator) (*corev1.Pod, error) {
	if len(nrop.Status.NodeGroups) < 1 {
		return nil, errors.New("node groups not reported, nothing to do")
	}
	// nrop places all daemonsets in the same namespace on which it resides, so any group is fine
	namespace := nrop.Status.NodeGroups[0].DaemonSet.Namespace // shortcut
	klog.InfoS("NROP pod", "namespace", namespace)

	sel, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{
		MatchLabels: map[string]string{
			"app": "numaresources-operator",
		},
	})
	if err != nil {
		return nil, err
	}
	klog.InfoS("NROP pod", "selector", sel.String())

	podList := corev1.PodList{}
	err = cli.List(ctx, &podList, &client.ListOptions{Namespace: namespace, LabelSelector: sel})
	if err != nil {
		return nil, err
	}

	if len(podList.Items) != 1 {
		return nil, fmt.Errorf("unexpected number of pods found: %d", len(podList.Items))
	}

	return &podList.Items[0], nil
}

@rbaturov
Copy link
Contributor Author

once #1111 lands, we can reimplement the version check using the info bundled in the operator container image, avoiding any extra deps. To find the operator pod we can add something

$ cat test/utils/deploy/find.go 
/*
 * Copyright 2024 Red Hat, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package deploy

import (
	"context"
        "errors"
	"fmt"

	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/klog/v2"

	"sigs.k8s.io/controller-runtime/pkg/client"

	nropv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1"
)

func FindNUMAResourcesOperatorPod(ctx context.Context, cli client.Client, nrop *nropv1.NUMAResourcesOperator) (*corev1.Pod, error) {
	if len(nrop.Status.NodeGroups) < 1 {
		return nil, errors.New("node groups not reported, nothing to do")
	}
	// nrop places all daemonsets in the same namespace on which it resides, so any group is fine
	namespace := nrop.Status.NodeGroups[0].DaemonSet.Namespace // shortcut
	klog.InfoS("NROP pod", "namespace", namespace)

	sel, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{
		MatchLabels: map[string]string{
			"app": "numaresources-operator",
		},
	})
	if err != nil {
		return nil, err
	}
	klog.InfoS("NROP pod", "selector", sel.String())

	podList := corev1.PodList{}
	err = cli.List(ctx, &podList, &client.ListOptions{Namespace: namespace, LabelSelector: sel})
	if err != nil {
		return nil, err
	}

	if len(podList.Items) != 1 {
		return nil, fmt.Errorf("unexpected number of pods found: %d", len(podList.Items))
	}

	return &podList.Items[0], nil
}

@ffromani I'm not sure how I should use the buildInfo utility with this

@ffromani
Copy link
Member

the idea would be to use a function like this to find the operator pod (from the test) and once in the operator pod trivially query (aka cat) buildinfo.json to learn how the operator was built without needing external libs, OLM access etc. etc.

@rbaturov
Copy link
Contributor Author

the idea would be to use a function like this to find the operator pod (from the test) and once in the operator pod trivially query (aka cat) buildinfo.json to learn how the operator was built without needing external libs, OLM access etc. etc.

Thanks, much more clear now

@rbaturov rbaturov changed the title e2e: introduce upgrade suite WIP: e2e: introduce upgrade suite Dec 16, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2024
@rbaturov rbaturov force-pushed the post-upgrade-test branch 3 times, most recently from 3b17e71 to 85e7efe Compare December 17, 2024 08:55
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2024
@rbaturov rbaturov changed the title WIP: e2e: introduce upgrade suite e2e: introduce upgrade suite Dec 23, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2024
This suite is needed to verify NROP machineconfigs are removed after an upgrade.
Starting in version 4.18, NROP MachineConfigs containing the custom SELinux policy are expected to be removed unless a specific annotation is set in the NUMAResourcesOperator CR to enforce the use of the custom (legacy) SELinux policy. To ensure this behavior, we added a test that verifies MachineConfigs are removed when the annotation is absent in the CR.

Signed-off-by: Ronny Baturov <[email protected]>
The upgrade suite is only supported on operator versions 4.18 or newer
* Added a util func to find the operator pod.
* Utilized the buildinfo.json inside the container to inspect the operator version.

Signed-off-by: Ronny Baturov <[email protected]>
Copy link
Contributor

openshift-ci bot commented Dec 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, rbaturov, Tal-or

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 24, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 549a47c into openshift-kni:main Dec 24, 2024
15 checks passed
@rbaturov
Copy link
Contributor Author

/cherry-pick release-4.18

@openshift-cherrypick-robot

@rbaturov: cannot checkout 4.18: error checking out "4.18": exit status 1 error: pathspec '4.18' did not match any file(s) known to git

In response to this:

/cherry-pick 4.18

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-sigs/prow repository.

@openshift-cherrypick-robot

@rbaturov: new pull request created: #1129

In response to this:

/cherry-pick release-4.18

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants