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.

Sign up for GitHub

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

Jump to bottom

Replace kubectl apply in e2e test framework #9696

Open
killianmuldoon opened this issue Nov 9, 2023 · 14 comments · May be fixed by #10442
Open

Replace kubectl apply in e2e test framework #9696

killianmuldoon opened this issue Nov 9, 2023 · 14 comments · May be fixed by #10442
Assignees
@adilGhaffarDev @cahillsf
Labels
area/e2e-testing Issues or PRs related to e2e testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Nov 9, 2023 •
edited

In the CAPI e2e test framework currently the kubectl binary is called to apply yamls when creating Clusters. This would improve error reporting from this function which may help to debug #9688.

cluster-api/test/framework/cluster_proxy.go

Lines 248 to 254 in 2caa534

// Apply wraps `kubectl apply ...` and prints the output so we can see what gets applied to the cluster.
func (p *clusterProxy) Apply(ctx context.Context, resources []byte, args ...string) error {
Expect(ctx).NotTo(BeNil(), "ctx is required for Apply")
Expect(resources).NotTo(BeNil(), "resources is required for Apply")
return exec.KubectlApply(ctx, p.kubeconfigPath, resources, args...)
}

The easiest solution is to create objects in the test framework with a client.Create call either changing the underlying function. This is the approach in #9077. We can ignore alreadyExists errors to make the call retryable.

Alternatively we could use the actual rest call from kubectl which has the benefit of actually being an 'apply' call. The only issue with this is needing to import the k8s.io/kubectl library and it being relatively complicated to implement.
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 9, 2023
@killianmuldoon
Copy link
Contributor Author

My preference for a quick win is to just replace the Apply calls with Create calls - we can also deprecate the existing functions with call kubectl.

If somebody specifically wants the apply functionality in the e2e framework it would be a good opportunity to contribute.

@killianmuldoon
Copy link
Contributor Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 9, 2023
@sbueringer
Copy link
Member

At a first glance we should get rid of all the os.exec kubectl stuff (not sure if there's more than apply)

@killianmuldoon
Copy link
Contributor Author

There's also a Wait in there which is unused and easy to remove.

@sbueringer sbueringer added this to the v1.7 milestone Nov 14, 2023
@sbueringer
Copy link
Member

A first implementation can be found here: #9695

/help

@k8s-ci-robot
Copy link
Contributor

@sbueringer:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

A first implementation can be found here: #9695

/help

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.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Nov 14, 2023
@sbueringer sbueringer added area/testing Issues or PRs related to testing area/e2e-testing Issues or PRs related to e2e testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. area/testing Issues or PRs related to testing labels Nov 14, 2023
@adilGhaffarDev
Copy link
Contributor

@sbueringer @killianmuldoon I can take this issue.

@sbueringer
Copy link
Member

Thx!

/assign adilGhaffarDev

This was referenced Nov 20, 2023
Closed
Closed
@cahillsf
Copy link
Member

cahillsf commented Mar 5, 2024

👋 @killianmuldoon touching back on this one -- should this issue still be pursued?

i see one of the PRs to fix was closed with a comment indicating a separate PR was merged that covered this:
#9731 (comment)

@sbueringer sbueringer removed this from the v1.7 milestone Mar 12, 2024
@sbueringer
Copy link
Member

I think if we find a good way to get rid of the os.exec with kubectl that would be good

@fabriziopandini
Copy link
Member

/kind cleanup
/priority backlog

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Apr 11, 2024
@cahillsf cahillsf linked a pull request Apr 16, 2024 that will close this issue
Draft
@cahillsf
Copy link
Member

/assign (DMed you about this awhile ago @adilGhaffarDev taking a stab at this one)

@cahillsf
Copy link
Member

/assign cahillsf

@cahillsf
Copy link
Member

@sbueringer 👋 i have a draft open for this (sloppy right now but want to make sure im going in the right direction)

the current signature accepts this args ...string parameter but the only actual arg that is in use for the e2e tests is --selector.

the current logic of my update:

  • parses the selector into a label matcher (defaulting to all if no selector passed)
  • if the object doesn't exist Create it
  • else, Update it

is this an approach that makes sense?

if so I was thinking of updating the signature to something like:
func (p *clusterProxy) CreateOrUpdate(ctx context.Context, resources []byte, labelSelectors ...string) error

is there any reason we would need to support other kubectl args if they are not currently in use? any other thoughts/ideas as to how i should move forward also appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adilGhaffarDev adilGhaffarDev

@cahillsf cahillsf

Labels
area/e2e-testing Issues or PRs related to e2e testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🌱 WIP replace exec with client create cahillsf/cluster-api
6 participants
@sbueringer @fabriziopandini @k8s-ci-robot @adilGhaffarDev @killianmuldoon @cahillsf