e2e: serial: Test for performance profile update and NRT reconciliation #1255
e2e: serial: Test for performance profile update and NRT reconciliation #1255SargunNarula wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SargunNarula The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
shajmakh
left a comment
shajmakh
left a comment
There was a problem hiding this comment.
Thanks! below is the initial review
| newTMPolicy := getNewTopologyManagerPolicyValue(initialTMPolicy) | ||
| Expect(initialTMPolicy).ToNot(Equal(newTMPolicy), "new TM policy should differ from the initial one") | ||
|
|
||
| By("retrieving the PerformanceProfile configmap from the management cluster") |
There was a problem hiding this comment.
we need to do extra verification layer before, how do you know if the configmap is for the PP ? and whether NROP is using PP config or kubeletconfig?
There was a problem hiding this comment.
Since the ConfigMap is retrieved from the tuningConfig field under the NodePool spec, it effectively serves as the source of the PerformanceProfile configuration. In the NROP-HCP environment, there is no separate KubeletConfig as far as I’m aware. On the hosted cluster, the KubeletConfig ConfigMap is generated by the HyperShift operator and is marked as immutable: true.
There was a problem hiding this comment.
so is performance-profile is a must in HCP? what is the user want to do kubeletchanges but not involve PP? can't this just be done via kubeletconfig like in OCP?
SargunNarula
force-pushed
the
pp_hcp_test
branch
from
a47214f to
7b6837f
Compare
openshift-merge-robot
added
the
needs-rebase
SargunNarula
force-pushed
the
pp_hcp_test
branch
from
7b6837f to
e023c87
Compare
openshift-merge-robot
removed
the
needs-rebase
SargunNarula
force-pushed
the
pp_hcp_test
branch
from
e023c87 to
5b7afc1
Compare
|
/retest |
| Expect(modifiedYaml).ToNot(Equal(yamlData), "no changes applied to ConfigMap YAML") | ||
|
|
||
| targetCM.Data["tuning"] = modifiedYaml | ||
| err = e2eclient.MNGClient.Update(context.TODO(), targetCM) |
There was a problem hiding this comment.
If I'm correct the reboot should happen on the update, where are we waiting for reboot?
There was a problem hiding this comment.
Yes, the node reboots on a PerformanceProfile update, but we don’t wait for the reboot explicitly. Since NRT relies on the RTE DaemonSet, and RTE pods are recreated only after the node becomes Ready, waiting for the updated NRT implicitly covers the reboot. Please let me know if I’ve misunderstood anything here.
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
I didn't went fully into reviewing the code itself, but there's an hidden assumption here, which is that a performance profile is available for the test.
This is not always the case because one can create a kubeletconfig directly.
The only valid assumption is that we have a kubeleconfig available, not a performance profile.
…reconciliation Signed-off-by: Sargun Narula <snarula@redhat.com>
|
@SargunNarula: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
Tal-or
left a comment
Tal-or
left a comment
There was a problem hiding this comment.
LGTM overall.
Please rebase if still relevant.
| return ret | ||
| } | ||
|
|
||
| func getNewTopologyManagerPolicyValue(oldTopologyManagerPolicyValue string) string { |
There was a problem hiding this comment.
I would rename it getDifferentTopologyManagerPolicyFrom(oldTopologyManagerPolicy string) string
| sampleNode corev1.Node | ||
| } | ||
|
|
||
| type Tuning struct { |
There was a problem hiding this comment.
Worth checking if we already importing hypershift APIs
| Expect(schedOK).To(BeTrue(), "pod %s/%s not scheduled with expected scheduler %s", updatedPod.Namespace, updatedPod.Name, serialconfig.SchedulerTestName) | ||
| }) | ||
|
|
||
| It("[test_id:80821] Verify Performance Profile updates on management cluster reflect in NRT resources", Label("reboot_required", label.Slow, label.HyperShift), func() { |
There was a problem hiding this comment.
Since we don't have the dependencies for NTO here, In the serial suite we've added a while ago a dynamicClient to get the performanceprofile. It's being used here
It's possible this test can use that.
5 participants
This PR implements the test for the performance profile update over the management cluster and verifies the NRT objects reconciliation with the updated changes on the hosted cluster.