Add ExecInDeployment method

[keeprocking]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This patch adds the ExecInDeployment method, which works similarly to

$ kubectl exec -n <namespace> -it deploy/<deployment> -- <command>

It's useful when you need to run a command from one of the pods belonging to a specific deployment, but you don't care which one.

For example, suppose there's a deployment running a custom controller, and you need to assert metrics written by that controller. The metrics endpoint is exposed via a Service, so it doesn't really matter which pod you call it from.

Instead of manually retrieving the list of pods belonging to the deployment, selecting one and passing it to ExecInPod, you could do this:

var stdout, stderr *bytes.Buffer
cmd := []string{"wget", "-qO-", fmt.Sprintf("http://my-service.%s.svc:8080/metrics", c.Namespace())}
if err := c.Client().Resources().ExecInDeployment(ctx, c.Namespace(), deployment.Name, cmd, &stdout, &stderr); err != nil {
  t.Log(stderr.String())
  t.Fatal(err)
}
metrics := stdout.String()

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added the Resources.ExecInDeployment method, which finds a deployment by name and runs the command in the first container of the first pod in that deployment

Additional documentation e.g., Usage docs, etc.:

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: keeprocking / name: Ivan Mukhin (3847fece9db652f839bdd5a392c8809b8c5b2a47, 0914c0854c8270bb1e6db43d58f4ec3de9588c80, f30409352732e654bd96472f7b3d77aa3d02e895, 8a0e8d19fb599814060dd4afa0abadb7b5fafea0)

[k8s-ci-robot]

Welcome @keeprocking!

It looks like this is your first PR to kubernetes-sigs/e2e-framework 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/e2e-framework has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @keeprocking. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[keeprocking]

Hey @vladimirvivien, could you please take a look? Thanks.

[vladimirvivien]

Thanks for pinging me directly. I will take a look soon.

[keeprocking]

Hey @vladimirvivien, sorry for the delay — been wrapping things up at work before the long-awaited three-week vacation.

I like the idea of making the new API more general. However, I can't help noticing that the suggested approach isn't very extension-friendly. If we search the pod by index, why can't we do the same for the container? Or, for example, search the pod by the name prefix rather than selecting it by index?

I'd suggest using the functional options pattern. That way, we can easily extend the API later if needed:

// Start with this
WithPodIndex(int) // Selects the pod with the given index

// Probably add these later
WithPodPrefix(string) // Selects the first pod whose name starts with the given prefix
WithContainerName(string) // Selects the container whose name fully matches the given name

The new API signature would be:

func (r *Resources) ExecInDeployment(ctx context.Context, namespaceName, deploymentName string, command []string, stdout, stderr *bytes.Buffer, opts ...ExecInDeploymentOption) error

Another advantage of this approach is that my initial implementation (select the first container of the first pod) works out of the box, with no additional options.

I still don't like the idea of passing this many arguments to a single function, but I was trying to keep it consistent with the ExecInPod signature. I don't see a cleaner way to handle this without some significant refactoring — which I have a few thoughts about, but maybe later.

I've already made a POC in https://github.com/kubernetes-sigs/e2e-framework/pull/506/commits/0914c0854c8270bb1e6db43d58f4ec3de9588c80 — let me know if it's viable.

Cheers.

[vladimirvivien]

@keeprocking I left some comments. Please take a look.

[keeprocking]

Hi, sorry for the delay — got a lot going on in my life right now.

I'm working on the suggested changes, but they do require some time to get it done right.

[keeprocking]

@vladimirvivien it definitely took me much longer than I expected!

While I like the suggested approach, I can't help but notice that fields like containerName and containerIdx in the proposed deploymentOptions struct are mutually exclusive. You can't select a container by both index and name at the same time. This makes the pod/container selection process more difficult, as it's not clear which field should take priority.

After scratching my head for quite a while, I came up with 3847fece9db652f839bdd5a392c8809b8c5b2a47. I believe this is as extensible as it can get:

• the selection process is clear and straightforward;
• we have shortcuts for selecting pods and containers by index or name, which should cover the vast majority of cases;
• additionally, there are predicate-based selection rules for more fine-grained control when needed. These operate on resource objects (v1.Pod and v1.Container) and cover more exotic cases;
• selection logic is based on generic predicates, so it can be extended with new resource types (and possibly reused in other parts of the framework).

I don't quite understand, though, what the ListOption you mentioned is for. We already have a label selector (coming from a deployment) for getting a list of pods — isn't that enough? We don't use ListOption when fetching a deployment since it's a get and not a list operation, so how is it supposed to be used?

Let me know what you think and thanks for the patience.

[keeprocking]

@vladimirvivien could you have a look, please?

[cpanato]

seems good /lgtm /hold

defer to @vladimirvivien as have comments

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: keeprocking, vladimirvivien

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~~ [vladimirvivien]

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

[cpanato]

/unhold