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

Addon update heapster #5199

Merged
merged 2 commits into from
Sep 21, 2018
Merged

Addon update heapster #5199

merged 2 commits into from
Sep 21, 2018

Conversation

recollir
Copy link
Contributor

Updated heapster version and added a note about heapster being deprecated.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 24, 2018
@mikesplain
Copy link
Contributor

Thanks @recollir!

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added 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 May 24, 2018
@recollir
Copy link
Contributor Author

/assign @KashifSaadat

@mikesplain
Copy link
Contributor

@recollir Looks like you've got a conflict, mind rebasing?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2018
@recollir
Copy link
Contributor Author

recollir commented Jun 1, 2018

/test pull-kops-e2e-kubernetes-aws

@recollir
Copy link
Contributor Author

recollir commented Jun 1, 2018

@mikesplain rebased. but pull-kops-e2e-kubernetes-aws is flakey again

command:
- /heapster
- --source=kubernetes.summary_api:''
- image: k8s.gcr.io/addon-resizer:2.1
Copy link
Member

Choose a reason for hiding this comment

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

What's the source for this? I'm trying to make sense of the version of addon-resizer - 2.1 seems older than 1.8.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find the source to this, what appeared to be, "newer" version. I will revert it back to the now 1.8.3

@justinsb justinsb added this to the 1.10 milestone Jun 2, 2018
Copy link
Contributor

@KashifSaadat KashifSaadat left a comment

Choose a reason for hiding this comment

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

LGTM mostly, just one small change in regards to the version of addon-resizer (recommended version in docs and the current latest is v1.8.1), unless v2.1 is specifically required for some reason?

@KashifSaadat
Copy link
Contributor

Hey @recollir, just wondering if you've had a chance to review the last outstanding comment for this PR in regards to the addon-resizer version?

@recollir
Copy link
Contributor Author

I was out travelling. Will look at it during the next few days.

@justinsb justinsb modified the milestones: 1.10, 1.11 Jul 18, 2018
@justinsb
Copy link
Member

Would love to have this in, but moving it to the 1.11 milestone as 1.10 is imminent

@recollir
Copy link
Contributor Author

I can't find the source to this, what appeared to be, "newer" version. I will revert it back to the now 1.8.3

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2018
@recollir
Copy link
Contributor Author

I have now reverted the addon-resizer version to 1.8.3 as that version was used in https://github.com/kubernetes/kubernetes/blob/c65f65cf6aea0f73115a2858a9d63fc2c21e5e3b/cluster/addons/cluster-monitoring/google/heapster-controller.yaml#L82

Sorry for the delay. We had a good long summer in Europe with long vacations and travelling.

@KashifSaadat
Copy link
Contributor

Hey @recollir, no problem thanks for making the update :)

The test failures have been fixed in master, you might need to rebase your branch off master for them to disappear.

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2018
@justinsb
Copy link
Member

Thanks for the version tweaks @recollir

/approve
/lgtm

Going to force-merge as CI is just upset about the spellchecker and not related to this PR.

@justinsb justinsb merged commit 2db17b3 into kubernetes:master Sep 21, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, KashifSaadat, mikesplain, recollir

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:
  • OWNERS [KashifSaadat,justinsb,mikesplain]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@so0k
Copy link
Contributor

so0k commented Nov 1, 2018

this seems to have broken this addon for us:

Nov 01 06:14:44 ip-10-61-92-234 docker[2432]: error loading channel "https://raw.githubusercontent.com/kubernetes/kops/master/addons/monitoring-standalone/addon.yaml": error parsing addons: error converting YAML to JSON: yaml: line 22: did not find expected key
Nov 01 06:14:44 ip-10-61-92-234 docker[2432]: W1101 06:14:44.412313    2466 kube_boot.go:157] error applying channel "monitoring-standalone": error running channels: exit status 1

forking for our 1.9 clusters :/

@KashifSaadat
Copy link
Contributor

Hi @so0k, it looks like the indentation was slightly out in the addon.yaml file. I've fixed this in PR #6032.

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. 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.

None yet

6 participants