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

kubeadm: use skeleton as provider #10913

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

neolit123
Copy link
Member

The e2e framework deprecated unknown providers in 1.13 and in 1.14
this support will be removed.

Move the kubernetes-anywhere based jobs to "skeleton" as provider.

ref kubernetes/kubernetes#70200

/priority important-soon
/kind cleanup
/area config
/assign @krzyzacy
cc @pohly

The e2e framework deprecated unknown providers in 1.13 and in 1.14
this support will be removed.

Move the kubernetes-anywhere based jobs to "skeleton" as provider.
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/config Issues or PRs related to code in /config size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 23, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2019
@BenTheElder
Copy link
Member

you may want to use "local" as that will still attempt the SSH log dumping hacks IIRC

/cc @dims

@k8s-ci-robot k8s-ci-robot requested a review from dims January 23, 2019 23:29
@neolit123
Copy link
Member Author

you may want to use "local" as that will still attempt the SSH log dumping hacks IIRC

we tried "local" last time and it started failing. we want the bare bone variant (skeleton).
#9919

@BenTheElder
Copy link
Member

BenTheElder commented Jan 23, 2019 via email

@neolit123
Copy link
Member Author

IMHO, we should be deprecating the usage of log dumping and other scripts from the cluster folder. that was the cause of failure when we used "local" and we couldn't figure out why.

@neolit123
Copy link
Member Author

/assign @timothysc

@timothysc
Copy link
Member

/approve
/lgtm

It seems like we've created a weird tapestry of dependencies on provider. Is there a test issue log'd b/c there is a conflating constraint on tests<>test-infra. When we built sonobuoy we decoupled that.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d91f4ff58ccc54f7441fe80503dc6f3668b9837f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2019
@k8s-ci-robot k8s-ci-robot merged commit 130cdda into kubernetes:master Jan 24, 2019
@k8s-ci-robot
Copy link
Contributor

@neolit123: Updated the job-config configmap using the following files:

  • key kubeadm-upgrade.yaml using file config/jobs/kubernetes/sig-cluster-lifecycle/kubeadm-upgrade.yaml
  • key kubeadm-x-on-y.yaml using file config/jobs/kubernetes/sig-cluster-lifecycle/kubeadm-x-on-y.yaml
  • key kubeadm.yaml using file config/jobs/kubernetes/sig-cluster-lifecycle/kubeadm.yaml

In response to this:

The e2e framework deprecated unknown providers in 1.13 and in 1.14
this support will be removed.

Move the kubernetes-anywhere based jobs to "skeleton" as provider.

ref kubernetes/kubernetes#70200

/priority important-soon
/kind cleanup
/area config
/assign @krzyzacy
cc @pohly

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.

@neolit123
Copy link
Member Author

@timothysc

It seems like we've created a weird tapestry of dependencies on provider. Is there a test issue log'd b/c there is a conflating constraint on tests<>test-infra. When we built sonobuoy we decoupled that.

kubetest transparently passes a provider to the e2e FW, like aws, gce etc.
with the changes that @pohly is planning for this cycle any uknown provider will error-out.

in terms of skeleton this is definitely the type we want to use for k-a and future tests using kind.
local on the other hand causes weird bugs from the /cluster folder that nobody knows how to fix and also it is managing things that we don't need for k-a and kind.

@BenTheElder
Copy link
Member

Yeah, echoing @neolit123's comment: we need skeleton regardless of anything in test-infra to make the current e2e.test code not blow up on an unknown provider -- IIRC that is a special value meaning "no cloud provider logic in the tests please", which we need for EG kind.

local is essentially the same thing but actually comes with extra baggage attached, in particular the behavior where e2e.test shells out to some bash script(s) in cluster/ to attempt to dump logs with a lot of provider special cased logic, environment variables, etc. again.

With kind's integration we're attempting to not depend on any of that legacy cruft so we set skeleton, and implement log dumping outside of e2e.test entirely.

Personally I'd just as soon not have e2e.test be aware of providers or even have this flag, period, but I don't think that's likely, or particularly pragmatic at the moment.

@timothysc
Copy link
Member

I'd like to flip the tests to know nothing at all about providers and instead search for "capabilities" and abstract away the notion of providers from the tests. This will take a concerted effort to clean up the tests in the long haul, but passing providers in is a hack/anti-pattern that we have lived with for too long.

@neolit123
Copy link
Member Author

if a null (skeleton is a null) provider is the default option - i.e. no treatment.
we can simply omit passing --provider=skeleton everywhere - as in "a job is not interested in a provider".
cc @pohly

@BenTheElder
Copy link
Member

I'd like to flip the tests to know nothing at all about providers and instead search for "capabilities" and abstract away the notion of providers from the tests. This will take a concerted effort to clean up the tests in the long haul, but passing providers in is a hack/anti-pattern that we have lived with for too long.

Agree that this is ideal, it will probably take longer though. In the meantime we've got some more things using then null ("skeleton") provider.

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. area/config Issues or PRs related to code in /config cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants