OCPBUGS-7980: e2e:wait: return updated pod object explicitly

[Tal-or]

In some of the tests we're implicitly relaying on the fact that the pod data returned from the pods.WaitFor* functions is the most updated one.

This is not the case in some of the functions on which we are storing the data on different pointer to pod object and never return it.

Instead of using different pod object, we should use the same one passed into the function.

[Tal-or]

Thank you @ffromani for helping with this

[ffromani]

The intent of the change is fine.

I'm reluctant to merge as-is as I'm generally against mutating arguments if we can help it. I'd prefer to return the updated object, even if this change is more invasive, I think is also clearer in the long run and lead to better code.

Also note that by far the most likely reason we have a pointer to pod as first argument (which will indeed mean the function can mutate the argument) is that we were concerned about performance in argument passing.

IOW, we pass a pointer not because the function was meant to mutate (that was never really documented or decided) but as optimization.

[ffromani]

/cc @MarSik @yanirq @swatisehgal

[Tal-or]

the intent of the change is fine.

I'm reluctant to merge as-is as I'm generally against mutating arguments if we can help it. I'd prefer to return the updated object, even if this change is more invasive, I think is also clearer in the long run and lead to better code.

Also note that by far the most likely reason we have a pointer to pod as first argument (which will indeed mean the function can mutate the argument) is that we were concerned about performance in argument passing.

IOW, we pass a pointer not because the function was meant to mutate (that was never really documented or decided) but as optimization.

Yes I completely understand your intention.
I mutate the pointer itself because:

1. I wanted to avoid changing the functions signature, because as you said it would required more invasive change (maybe in cnf-feature-deploy as well?)
2. mutating the pod pointer is common and a controller-runtime-like approach.

Anyway I'm OK with what ever approach

[ffromani]

1. I wanted to avoid changing the functions signature, because as you said it would required more invasive change (maybe in cnf-feature-deploy as well?)

2. mutating the pod pointer is common and a controller-runtime-like approach.

You make good points. Even if I'm not a fan of this approach, I won't block it either. I've tagged the other reviewers, let's see if we have a preference collectively.

[Tal-or]

/test e2e-hypershift

[MarSik]

I wish we had an explicit way of marking the pointer as mutable (&mut Pod from Rust anyone? :)
I am OK with the approach, but I want to see that written in the doctext for the each of the functions.

[Tal-or]

I wish we had an explicit way of marking the pointer as mutable (&mut Pod from Rust anyone? :) I am OK with the approach, but I want to see that written in the doctext for the each of the functions.

const pod *corev1.Pod :)

[yanirq]

1. I wanted to avoid changing the functions signature, because as you said it would required more invasive change (maybe in cnf-feature-deploy as well?)

2. mutating the pod pointer is common and a controller-runtime-like approach.

You make good points. Even if I'm not a fan of this approach, I won't block it either. I've tagged the other reviewers, let's see if we have a preference collectively.

I would not rule out the "invasive" approach yet , can we check to what extent is it invasive ? (maybe its not that large)

[ffromani]

Rough guesstimation:

$ git log -1
commit 0e3f9fca2f798654e3a0e8758241a299b27dae8d (HEAD -> master, upstream/release-4.15, upstream/release-4.14, upstream/master)
Author: SargunNarula <57625241+SargunNarula@users.noreply.github.com>
Date:   Wed Aug 2 18:14:03 2023 +0530

    e2e: remove image parameter from must gather (#743)
$ git grep -w WaitForDeletion | grep -v vendor | grep pods | wc -l
7
$ git grep -w WaitForCondition | grep -v vendor | grep pods | wc -l
15
$ git grep -w WaitForPhase | grep -v vendor | grep pods | wc -l
3

[swatisehgal]

My thought on this is that for operations like the ones here (WaitForDeletion, WaitForCondition and WaitForPhase) it seems inappropriate to assume that the pod object returned is mutated (and is fresher than the object passed). Mutation should not be part of the equation here and the focus should be on waiting until the pod is deleted, meets a condition or attains a certain phase respectively. Ideally we should have had a function definition where we passed pod name instead of a pointer to pod object and that would have made it explicit that we don't expect pod object to be mutated.

If the caller relies on a fresh pod object, they should obtain it explicitly. The current changes seem to be further enforcing this (incorrect?) assumption rather than correcting or making it explicit. Now the question is are there many places where we have this assumption? IOW, what is the impact surface if we wanted to rectify this assumption of obtaining upto date pod objects when these functions are called?

If the motivating factor here to ensure clarity and to facilitate the consumer of utils/pods package, how about we add a function to explicitly obtain an updated pod object like below and use that where we expect to have a fresh pod object:

func GetPod(podName, namespace string) (*corev1.Pod, error){
.....
}

I am curious about what exactly is the reason we want to have the latest pod object? Do we expect the pod object to change here? In theory I can see a few use cases (in place VPA allowing resources updates, updates to labels, taints, tolerations etc) but I would like to understand it better in the context of the use cases we care about and our test suite.

[ffromani]

My thought on this is that for operations like the ones here (WaitForDeletion, WaitForCondition and WaitForPhase) it seems inappropriate to assume that the pod object returned is mutated (and is fresher than the object passed). Mutation should not be part of the equation here and the focus should be on waiting until the pod is deleted, meets a condition or attains a certain phase respectively. Ideally we should have had a function definition where we passed pod name instead of a pointer to pod object and that would have made it explicit that we don't expect pod object to be mutated.

I fully agree. It would be much better if the functions accept the pod identification data (= ObjectKey or namespace/name pair) rather than the full pod object.

If the caller relies on a fresh pod object, they should obtain it explicitly. The current changes seem to be further enforcing this (incorrect?) assumption rather than correcting or making it explicit. Now the question is are there many places where we have this assumption? IOW, what is the impact surface if we wanted to rectify this assumption of obtaining upto date pod objects when these functions are called?

This would be indeed the most explicit approach.

If the motivating factor here to ensure clarity and to facilitate the consumer of utils/pods package, how about we add a function to explicitly obtain an updated pod object like below and use that where we expect to have a fresh pod object:

func GetPod(podName, namespace string) (*corev1.Pod, error){
.....
}

I am curious about what exactly is the reason we want to have the latest pod object? Do we expect the pod object to change here? In theory I can see a few use cases (in place VPA allowing resources updates, updates to labels, taints, tolerations etc) but I would like to understand it better in the context of the use cases we care about and our test suite.

It's to save a Get which is not really costly, but redundant (yes, this is a form of premature optimization)

In a nutshell, some Wait* functions return a copy of the object after the waited condition is met, effectively saving the caller a Get. This is both an optimization and a implementation detail leaking (The Wait* functions are all essentially a Get+condition check wrapped in a poll loop with a timeout), but the saving grace is that it still kinda fits in the semantic, albeit not cleanest approach.

[Tal-or]

Then it's settle we have a decision. please review that latest patch

[Tal-or]

/test e2e-gcp-pao
New flake/failure, that we won't investigate as part of this PR

[rfe_id:27368][performance] Verify API Conversions  [JustBeforeEach] when the GloballyDisableIrqLoadBalancing field set to false should preserve the value during the v1 <-> v2 conversion
  [JustBeforeEach] /go/src/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/performance.go:777
  [It] /go/src/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/performance.go:810
  [FAILED] Failed to create profile
  Unexpected error:
      <*errors.StatusError | 0xc00037f720>: {
          ErrStatus: {
              TypeMeta: {Kind: "", APIVersion: ""},
              ListMeta: {
                  SelfLink: "",
                  ResourceVersion: "",
                  Continue: "",
                  RemainingItemCount: nil,
              },
              Status: "Failure",
              Message: "Internal error occurred: failed calling webhook \"vwb.performance.openshift.io\": failed to call webhook: Post \"[https://performance-addon-operator-service.openshift-cluster-node-tuning-operator.svc:443/validate-performance-openshift-io-v2-performanceprofile?timeout=10s](https://performance-addon-operator-service.openshift-cluster-node-tuning-operator.svc/validate-performance-openshift-io-v2-performanceprofile?timeout=10s)\": no endpoints available for service \"performance-addon-operator-service\"",
              Reason: "InternalError",
              Details: {
                  Name: "",
                  Group: "",
                  Kind: "",
                  UID: "",
                  Causes: [
                      {
                          Type: "",
                          Message: "failed calling webhook \"vwb.performance.openshift.io\": failed to call webhook: Post \"https://performance-addon-operator-service.openshift-cluster-node-tuning-operator.svc:[443](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-node-tuning-operator/744/pull-ci-openshift-cluster-node-tuning-operator-master-e2e-gcp-pao/1688150438286725120#1:build-log.txt%3A443)/validate-performance-openshift-io-v2-performanceprofile?timeout=10s\": no endpoints available for service \"performance-addon-operator-service\"",
                          Field: "",
                      },
                  ],
                  RetryAfterSeconds: 0,
              },
              Code: 500,
          },
      }
      Internal error occurred: failed calling webhook "vwb.performance.openshift.io": failed to call webhook: Post "[https://performance-addon-operator-service.openshift-cluster-node-tuning-operator.svc:443/validate-performance-openshift-io-v2-performanceprofile?timeout=10s](https://performance-addon-operator-service.openshift-cluster-node-tuning-operator.svc/validate-performance-openshift-io-v2-performanceprofile?timeout=10s)": no endpoints available for service "performance-addon-operator-service"
  occurred
  In [JustBeforeEach] at: /go/src/github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/1_performance/performance.go:793 @ 08/06/23 12:22:20.221

[openshift-ci]

@Tal-or: all tests passed!

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.

[yanirq]

looks good to me . ill wait for others to give an ack before I add the approval

[ffromani]

/approve

thanks for the changes, IMO much better now

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, 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:

• OWNERS [ffromani]

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

[ffromani]

/hold

[ffromani]

holding to let other reviewers chime in

[ffromani]

/hold cancel

[openshift-ci-robot]

@Tal-or: Jira Issue OCPBUGS-7980 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

In some of the tests we're implicitly relaying on the fact that the pod data returned from the pods.WaitFor* functions is the most updated one.

This is not the case in some of the functions on which we are storing the data on different pointer to pod object and never return it.

Instead of using different pod object, we should use the same one passed into the function.

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.

[Tal-or]

/jira refresh

[openshift-ci-robot]

@Tal-or: Jira Issue OCPBUGS-7980: All pull requests linked via external trackers have merged:

• openshift/cluster-node-tuning-operator#729

Jira Issue OCPBUGS-7980 has been moved to the MODIFIED state.

In response to this:

/jira refresh

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.

[Tal-or]

/help

[Tal-or]

/cherry-pick ?

[openshift-cherrypick-robot]

@Tal-or: cannot checkout ?: error checking out ?: exit status 1. output: error: pathspec '?' did not match any file(s) known to git

In response to this:

/cherry-pick ?

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.