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

Remove support for CoreOS and Jessie #9065

Merged
merged 4 commits into from
May 28, 2020

Conversation

johngmyers
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2020
@k8s-ci-robot k8s-ci-robot added area/documentation area/nodeup size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 5, 2020
@johngmyers johngmyers force-pushed the remove-distro branch 2 times, most recently from 1a8e847 to 2642076 Compare May 5, 2020 05:52
docs/releases/1.18-NOTES.md Outdated Show resolved Hide resolved
nodeup/pkg/distros/identify.go Outdated Show resolved Hide resolved
@hakman
Copy link
Member

hakman commented May 5, 2020

This should also be removed:

// Only Debian releases newer than Jessie can install .deb via apt-get
// TODO: Refactor this function when Jessie support is dropped (duplicated code)
if t.HasTag(tags.TagOSDebianJessie) {
args = []string{"dpkg", "-i"}
} else {

And this:

TagOSDebianJessie = "_jessie"

@hakman
Copy link
Member

hakman commented May 5, 2020

I think this can also be removed:

if strings.Contains(ig.Spec.Image, "jessie") {
errString := fmt.Sprintf("%s cannot use machineType %s with image based on Debian jessie.", ig.Name, machineType)
allErrs = append(allErrs, field.Forbidden(fieldPath, errString))
continue
}

@hakman
Copy link
Member

hakman commented May 5, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2020
@rifelpet
Copy link
Member

rifelpet commented May 5, 2020

Nice work. I wonder if theres anything we can do to surface the error earlier, so that users that don't read the release notes aren't confused why their rolling-update cluster is failing to validate after the first node is being replaced.

I see awsValidateAMIforNVMe was using the name of the image which is far from complete coverage but I wonder if doing something similar would be "good enough" coverage to prevent the majority of support inquiries.

@hakman
Copy link
Member

hakman commented May 5, 2020

@rifelpet we have similar thing to instance type validation there. IMHO, we should do it in apply, where we can get the image name and owner account even for the AMI. Still I would do it in a subsequent patch.

@justinsb justinsb added this to the v1.19 milestone May 5, 2020
@johngmyers
Copy link
Member Author

@justinsb this PR is intended for 1.18. We shouldn't support these EOL distros for 1.18.

@hakman
Copy link
Member

hakman commented May 12, 2020

I think that we agreed during last office hours to include this in 1.18. Should this be merged as is, or there are any other issues to be addressed?

@justinsb
Copy link
Member

/milestone 1.18

@k8s-ci-robot
Copy link
Contributor

@justinsb: The provided milestone is not valid for this repository. Milestones in this repository: [backlog, v1.15, v1.16, v1.17, v1.18, v1.19]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.18

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.

@rdrgmnzs rdrgmnzs removed this from the v1.19 milestone May 22, 2020
@rdrgmnzs rdrgmnzs added this to the v1.18 milestone May 22, 2020
@justinsb
Copy link
Member

/milestone v1.18

@rdrgmnzs
Copy link
Contributor

Do we want to remove the references in the two files bellow in a separate PR?
kops/upup/pkg/fi/nodeup/nodetasks/service.go
cmd/kops/update_cluster.go

There's also a few references to CoreOS in the docs that we may want to cleanup separately to minimize any confusion.

Otherwise this LGTM.

@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 23, 2020
@johngmyers
Copy link
Member Author

Digital Ocean still defaults to a CoreOS image.

openstack.md says it defaults to CoreOS, but I don't see any code that makes this so.

Someone else will have to tackle docs/examples/coreos-kops-tests-multimaster.md

@hakman
Copy link
Member

hakman commented May 23, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2020
@hakman
Copy link
Member

hakman commented May 26, 2020

#9181 will address the default image for DO.

@rifelpet
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, rifelpet

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 May 28, 2020
@hakman
Copy link
Member

hakman commented May 28, 2020

/retest

3 similar comments
@johngmyers
Copy link
Member Author

/retest

@johngmyers
Copy link
Member Author

/retest

@hakman
Copy link
Member

hakman commented May 28, 2020

/retest

@johngmyers
Copy link
Member Author

The SIG-Storage flakes are strong today.
/retest

@hakman
Copy link
Member

hakman commented May 28, 2020

Feels like Jan all over again 😄

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/api area/documentation area/nodeup area/provider/aws Issues or PRs related to aws provider 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants