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

Complete support for Flatcar #7545

Merged
merged 4 commits into from
Dec 9, 2019
Merged

Conversation

mazzy89
Copy link
Contributor

@mazzy89 mazzy89 commented Sep 9, 2019

Signed-off-by: Salvatore Mazzarino [email protected]

Another tiny change that fixes the support for Flatcar (still broken, unfortunately)

As tested in the release 1.15.1-alpha.1

core@ip-172-20-65-153 ~ $ sudo journalctl -u coreos-cloudinit-350015389.service
-- Logs begin at Mon 2019-09-09 20:19:38 UTC, end at Mon 2019-09-09 20:26:45 UTC. --
Sep 09 20:19:59 ip-172-20-65-153.ec2.internal systemd[1]: Started Unit generated and executed by coreos-cloudinit on behalf of user.
Sep 09 20:19:59 ip-172-20-65-153.ec2.internal bash[1454]: == nodeup node config starting ==
Sep 09 20:19:59 ip-172-20-65-153.ec2.internal bash[1454]: Downloading nodeup (https://github.com/kubernetes/kops/releases/download/1.15.0-alpha.1/linux-am>
Sep 09 20:19:59 ip-172-20-65-153.ec2.internal bash[1454]:   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
Sep 09 20:19:59 ip-172-20-65-153.ec2.internal bash[1454]:                                  Dload  Upload   Total   Spent    Left  Speed
Sep 09 20:19:59 ip-172-20-65-153.ec2.internal bash[1454]: [158B blob data]
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal bash[1454]: [158B blob data]
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal bash[1454]: == Downloaded https://github.com/kubernetes/kops/releases/download/1.15.0-alpha.1/linux-amd64-no>
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal bash[1454]: Running nodeup
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal bash[1454]: nodeup version 1.15.0-alpha.1 (git-7c84c4848)
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal bash[1454]: F0909 20:20:00.545365    1481 distribution.go:96] unknown distribution: flatcar
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal systemd[1]: coreos-cloudinit-350015389.service: Main process exited, code=exited, status=1/FAILURE
Sep 09 20:20:00 ip-172-20-65-153.ec2.internal systemd[1]: coreos-cloudinit-350015389.service: Failed with result 'exit-code'.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 9, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @mazzy89. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 9, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 9, 2019
@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 9, 2019

Is there a way to test this safely?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 9, 2019

Following these two resources to test the changes locally

and make sure in this iteration that Flatcar is properly supported.

I'll get back here once I'll have it properly tested.

@justinsb
Copy link
Member

/ok-to-test

Thanks @mazzy89 - this lgtm. I'm going to mark as lgtm, but we can wait until you give the word before marking as approved (which would cause it to merge)

It's also fine to merge this now if you'd prefer, as it is fairly obviously correct - anything that references DistributionCoreOS should probably also reference FlatCar :-)

/lgtm

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 10, 2019
@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

@justinsb in order to test this locally, the only way to put this commit in the k8s.io/kops right? cause I can't build kops locally from my fork mazzy89/kops

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

I've figured out and starting to test it. fingers crossed

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

has the parseKubernetesVersion logic changed?
i’m trying to run my local built veersion ok kops and in .spec.KubernetesVersion i put the s3 bucket HTTP url but I get this:

E0910 11:40:50.428845   72526 versions.go:76] unable to parse Kubernetes version "https://<bucket>s3.amazonaws.com/kops/1.15.0-git.343cb85ed/"
error: error replacing cluster: spec.KubernetesVersion: Invalid value: "https://<bucket>.s3.amazonaws.com/kops/1.15.0-git.343cb85ed/": unable to determine kubernetes version

naturally the <bucket> points to an existing buckt

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2019
@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

/retest

@justinsb
Copy link
Member

@mazzy89 docs on how to do local development are here: docs/development/adding_a_feature.md

nodeup changes do require you to upload your own nodeup, but they should work!

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

@justinsb yeah I saw them. thank you. there is just an issue in handling the kubernetes version. when the kubernetesVersion points to a specific bucket like my case then it is important to append v to the version otherwise the function parseKubernetesVersion fails.

@justinsb
Copy link
Member

And the heuristics that pull a kubernetes version out of a URL expect it to contain /v1.15.0 (see ParseKubernetesVersion) - note the v. But in any case, you don't want to upload a custom kubernetes build, just a custom kops build - let me know if the instructions don't work for you!

But in any case, I think we can merge this PR now?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

Yes exactly. it expects /v<version> then I solved. now I'm stuck to

error building complete spec: error reading tag file "https://<bucket>.s3.amazonaws.com/kops/v1.15.0-git.9d23faef8/bin/linux/amd64/kube-apiserver.docker_tag": file does not exist

should I also upload the kbuernetes official build there?

I havne't fully tested this so not sure if this time it will work.

@justinsb
Copy link
Member

I think if you just specify the kubernetes version as 1.15.0 and it'll use the normal locations for kubernetes. If you want to mirror kubernetes to a private S3 bucket, you will have to copy the docker_tag files as well - and it's probably easiest just to copy everything!

But ... I don't think you need to mirror kubernetes to test this change?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

If I just specify a version like v1.15.0 in .spec.KubernetesVersion then how can I reference the nodeup that I've built locally and pushed to the S3 bucket?

I've followed this and indeed uploaded to my bucket https://github.com/kubernetes/kops/blob/master/docs/development/adding_a_feature.md#testing

@justinsb
Copy link
Member

Hmm ... thanks for pointing it out - the real instructions are a bit buried... The trick is the KOPS_BASE_URL env var, so it's this bit of the instructions that are most relevant:

export S3_BUCKET_NAME=<yourbucketname>
make kops-install dev-upload UPLOAD_DEST=s3://${S3_BUCKET_NAME}

KOPS_VERSION=`bazel run //cmd/kops version -- --short`
export KOPS_BASE_URL=https://${S3_BUCKET_NAME}.s3.amazonaws.com/kops/${KOPS_VERSION}/

Not sure if I should try to create another document about this or clean up the existing docs - any thoughts?

Also, the make push-aws-run or make bazel-push-aws-run trick can be handy for faster nodeup iteration!

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

but it's not the same setting up the KOPS_BASE_URL in the .spec.KubernetesVersion?

I've set up .spec.KubernetesVersion: https://${S3_BUCKET_NAME}.s3.amazonaws.com/kops/${KOPS_VERSION}/ and it fails because it does not find all the other assets like .dockertag, sha1 of the various components.

@justinsb
Copy link
Member

@mazzy89 ah no, they are different. KOPS_BASE_URL is the base url for the kops artifacts (nodeup, protokube). KubernetesVersion determines the base url for the kubernetes artifacts (kubelet, apiserver, kube-controller-manager, kube-scheduler, kube-proxy etc). KubernetesVersion can either be a full url (used when testing kubernetes) or a semver version (in which case we use the official distribution buckets for kubernetes).

@mazzy89
Copy link
Contributor Author

mazzy89 commented Sep 10, 2019

Oh now it makes sense. I was confused and I thought they were the same.

Do we have KOPS_BASE_URL as a spec in the Cluster CRD? or it is possible to pass it only via env variable?

@justinsb
Copy link
Member

Currently KOPS_BASE_URL can only be set by env-var. I don't really think of it as a cluster property (e.g. we don't have the kops version in the cluster config), but I could see that maybe we should support setting it in the configuration file

Signed-off-by: Salvatore Mazzarino <[email protected]>
Signed-off-by: Salvatore Mazzarino <[email protected]>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 24, 2019
@mazzy89
Copy link
Contributor Author

mazzy89 commented Nov 24, 2019

I've made the changes because I felt that would add less complexity and would help to move faster (hopefully) this PR.

So this PR will rely on the changes here #7978

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved
Signed-off-by: Salvatore Mazzarino <[email protected]>
@mazzy89
Copy link
Contributor Author

mazzy89 commented Dec 9, 2019

@hakman pushed its change and fixed the volume thing. I think now we can push this. It would be nice to have this as kops 1.15.1 so to fix CoreOS/Flatcar support.

cc @rifelpet @justinsb

@KashifSaadat
Copy link
Contributor

Hi @mazzy89, yep that sounds good. Thank you for the contribution!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KashifSaadat, mazzy89

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 Dec 9, 2019
@k8s-ci-robot k8s-ci-robot merged commit 852ed31 into kubernetes:master Dec 9, 2019
@linecolumn
Copy link

why not include it to v1.16alpha and v1.17alpha?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Dec 10, 2019

@linecolumn I've cherry-picked those changes and put into release-1.15 and release -1.17.

@hakman
Copy link
Member

hakman commented Dec 11, 2019

@mazzy89 just out of curiosity, why did you skip 1.16 with the cherry-picks?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Dec 11, 2019

Oh right forgot it.I was convinced i did but actually forgot it. thank you @hakman going to do that

k8s-ci-robot added a commit that referenced this pull request Dec 11, 2019
…origin-release-1.17

Automated cherry pick of #7545: Complete support for Flatcar
k8s-ci-robot added a commit that referenced this pull request Dec 11, 2019
…origin-release-1.15

Automated cherry pick of #7545: Complete support for Flatcar
@mazzy89
Copy link
Contributor Author

mazzy89 commented Dec 11, 2019

@hakman PR cherry-picked to 1.15/1.16/1.17 release branches. yours I saw it too. now hope to see a release pretty soon but considering the holidays I think the next release will be for Jan.

@hakman
Copy link
Member

hakman commented Dec 11, 2019

Hehe. Nice to see that it all worked out well.

k8s-ci-robot added a commit that referenced this pull request Dec 16, 2019
…origin-release-1.16

Automated cherry pick of #7545: Complete support for Flatcar
@mazzy89 mazzy89 deleted the flatcar-fix branch March 21, 2020 14:41
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

8 participants