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

cluster/aws: Clean up dhcp-options #38645

Merged
merged 1 commit into from
Dec 13, 2016
Merged

cluster/aws: Clean up dhcp-options #38645

merged 1 commit into from
Dec 13, 2016

Conversation

zmerlynn
Copy link
Member

What this PR does / why we need it: After adding the aws janitor, the thing we're consistently sweeping is the DhcpOptionSets created by cluster/aws/util.sh (and there were thousands on the first run). Fix it!

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):
Special notes for your reviewer:
Release note:

Fix an issue where AWS tear-down leaks an DHCP Option Set.

@k8s-reviewable
Copy link

This change is Reviewable

@zmerlynn zmerlynn added this to the v1.5 milestone Dec 12, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 12, 2016
@zmerlynn zmerlynn added cherrypick-candidate and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 12, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 12, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit f2d410f. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@zmerlynn
Copy link
Member Author

@k8s-bot cri node e2e test this

justinsb
justinsb reviewed Dec 12, 2016
View reviewed changes
cluster/aws/util.sh
@@ -562,6 +562,23 @@ function create-dhcp-option-set () {
echo "Using DHCP option set ${DHCP_OPTION_SET_ID}"
}

function delete-dhcp-option-sets () {
if [[ -n "${DHCP_OPTION_SET_ID:-}" ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why we check DHCP_OPTION_SET_ID.

Also maybe inline this function - it's a little less modularized, but it makes it easier to see that we're deleting consistently?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#27278 introduced setting DHCP_OPTION_SET_ID. Happy to inline, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - gotcha!

@zmerlynn
cluster/aws: Clean up dhcp-options
3e85983 
After adding the aws janitor, the thing we're consistently sweeping is
the DhcpOptionSets created by cluster/aws/util.sh (and there were
thousands on the first run). Fix it!
@zmerlynn
Copy link
Member Author

PTAL

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 12, 2016
@justinsb
Copy link
Member

LGTM - thanks!

@justinsb justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 17de686 into kubernetes:master Dec 13, 2016
@zmerlynn
Copy link
Member Author

@saad-ali: Ping on cherrypick request

@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 28, 2016
@saad-ali
Copy link
Member

@saad-ali: Ping on cherrypick request

In general, feel free to create the pull request PR, I use that as my signal to review the request. If for some reason we decide not to go forward with the cherry pick we can always close the PR.

@zmerlynn
Copy link
Member Author

zmerlynn commented Dec 28, 2016 •
edited
Loading

But .. the cherrypick dashboard was created to monitor these things, and so contributors can avoid fighting with bots after they put up the request? That's a process change, if we're not using it.

@zmerlynn zmerlynn mentioned this pull request Dec 28, 2016
Merged
@zmerlynn zmerlynn deleted the clean-dhcp-options branch December 28, 2016 22:52
@saad-ali
Copy link
Member

But .. the cherrypick dashboard was created to monitor these things, and so contributors can avoid fighting with bots after they put up the request? That's a process change, if we're not using it.

Ah, I should start using that :)

k8s-github-robot pushed a commit that referenced this pull request Dec 30, 2016
Merge pull request #39287 from zmerlynn/automated-cherry-pick-of-#386…
326cf6c 
…45-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #38645

Cherry pick of #38645 on release-1.5.

#38645: cluster/aws: Clean up dhcp-options
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

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

@justinsb justinsb justinsb left review comments

Assignees

@justinsb justinsb

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.
7 participants
@zmerlynn @k8s-reviewable @k8s-ci-robot @justinsb @k8s-github-robot @saad-ali @k8s-cherrypick-bot