Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

[stable/etcd-operator] Add cluster.restore, cluster.pod, nodeSelector & update #1200

Merged
lachie83 merged 8 commits into from
Aug 25, 2017
Merged

[stable/etcd-operator] Add cluster.restore, cluster.pod, nodeSelector & update #1200

lachie83 merged 8 commits into from
Aug 25, 2017

Conversation

@mikesplain
Copy link
Contributor

@mikesplain mikesplain commented May 30, 2017 •
edited
Loading

Added cluster.restore, cluster.pod, and nodeSelctors to both the etcd-operator pod and etcd cluster pods.

I've also updated the default version of the operator and etcd.

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

k8s-ci-robot commented May 30, 2017

Hi @mikesplain. 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 @k8s-bot 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.

Details

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. I understand the commands that are listed here.

@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 May 30, 2017
@mikesplain mikesplain force-pushed the etcd-operator_add_extra_args branch from 09ae305 to 8dd6e79 Compare May 30, 2017 18:40
@lachie83
Copy link
Contributor

lachie83 commented May 31, 2017

cc @chancez Thoughts on this?

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

@mikesplain Can you please share the reference of this being supported upstream in the etcd cluster definition?

@chancez
Copy link
Contributor

chancez commented Jun 6, 2017

I'm not sure about this. Why not just add the options that the operator supports via the TPR directly into the template?

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

Yeah that's what I would suggest too. Although I don't see any reference in the upstream example that we aren't yet covering

@mikesplain
Copy link
Contributor Author

mikesplain commented Jun 6, 2017 •
edited
Loading

@chancez @lachie83 There are a few examples here, as above. Currently spec.pod.* can't be set in this chart.

I don't mind switching it to add pod.* instead but I've seen quite a few of these types of options not present in charts (simply because software moves fast) so I figured by adding more of a catch all, it'll prevent having to open a PR every time a chart's application adds a new feature or flag.

@chancez
Copy link
Contributor

chancez commented Jun 6, 2017

@lachie83 it should basically cover all things in this struct: https://github.com/coreos/etcd-operator/blob/master/pkg/spec/cluster.go#L66 which in the example given, pod affinity is one thing to support, among others.

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

Thanks @mikesplain. Let me review and get back to you.

lachie83
lachie83 reviewed Jun 6, 2017
View reviewed changes
stable/etcd-operator/values.yaml Outdated
storageType: PersistentVolume
pv:
volumeSizeInMB: 512
extraArgs: {}
Copy link
Contributor

@lachie83 lachie83 Jun 6, 2017

Choose a reason for hiding this comment

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

Can you put reference to the upstream link with some examples and also add some commented out examples here?

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

Perfect thanks @chancez

@chancez
Copy link
Contributor

chancez commented Jun 6, 2017

And I'm still of the opinion that I'd rather see the helm chart explicitly enumerate available options rather than using the generic extraArgs, personally.

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

I tend to agree with @chancez . Can you scope that out in this PR?

@mikesplain
Copy link
Contributor Author

mikesplain commented Jun 6, 2017

@chancez @lachie83 I agree with your points, however would you mind having both? I don't mind adding the other args but I hoping to add an option to allow users to work around explicit options not being present.

@chancez
Copy link
Contributor

chancez commented Jun 6, 2017 •
edited
Loading

@mikesplain I personally think that that's best handled by forking the chart. I dislike trying to make charts overly extensible, because it means it can be difficult to handle upgrades.

If a user specifies an option in extraArgs because it's added to the operator, then we add that new option to the template, then it will be specified twice. This is bad for TPRs in particular because the schema isn't validated on submission, so the upgrade will succeed, but the TPR will be broken most likely.

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

I'm going to stand behind @chancez on this one.

Best way forward is to either fork and implement, or implement specific key vals.

How would you like to proceed @mikesplain

@mikesplain
Copy link
Contributor Author

mikesplain commented Jun 6, 2017

Alrighty, I'll modify this to just be those values then.

@lachie83
Copy link
Contributor

lachie83 commented Jun 6, 2017

Thanks @mikesplain . Appreciate your understanding

@mikesplain
Copy link
Contributor Author

mikesplain commented Jun 6, 2017

Absolutely, you guys know this stuff much better than I, I'm simply trying to help others work around road blocks I've come up against frequently as of late.

I am curious though, what's the intended path for forking @lachie83 @chancez?

Background: We use this chart as a dependency for one of our applications. When a developer finds a chart they want to use is missing a feature they need, they fork it, modify it and PR it back upstream. That's super easy, but the problem comes when you want to use the fork temporarily until the PR is merged, etc. Since dependency charts have to either be in the charts directory or in a chart repository as a package (based on my understanding), it requires forkers to setup a bit of other infrastructure to use (s3 or gsutil). I've setup https://github.com/technosophos/helm-plugins for us against our fork of this repo but it's kind of a hack... I was wondering if you have a suggested or best practice for forkers to utilize their forks without having to duplicate the merge and release process here? I appreciate your advice while I update the PR.

@chancez
Copy link
Contributor

chancez commented Jun 6, 2017

If I have to fork, I just change my dependency to reference the chart on my local file system using the file:// syntax. If you don't have control over the chart which has a dependency, then you fork that one too, and just modify it's dependency. It's not the best but it's not that bad either.

@mikesplain mikesplain force-pushed the etcd-operator_add_extra_args branch from 8dd6e79 to d4844d9 Compare June 21, 2017 18:11
@mikesplain mikesplain changed the title [stable/etcd-operator] Add extraArgs to cluster template [stable/etcd-operator] Add cluster.restore, cluster.pod and nodeSelector Jun 21, 2017
@mikesplain mikesplain changed the title [stable/etcd-operator] Add cluster.restore, cluster.pod and nodeSelector [stable/etcd-operator] Add cluster.restore, cluster.pod, nodeSelector & update Jun 21, 2017
@mikesplain
Copy link
Contributor Author

mikesplain commented Jun 21, 2017

@chancez @lachie83 I've updated the PR as suggested and added a few other things. Let me know if you have additional feedback.

@viglesiasce
Copy link
Contributor

viglesiasce commented Jul 21, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 21, 2017
@unguiculus
Copy link
Member

unguiculus commented Aug 13, 2017

Requires a chart version bump.

@viglesiasce @lachie83 Is this good to go?

@lachie83
Copy link
Contributor

lachie83 commented Aug 14, 2017

LGTM. Please bump the chart version

@mikesplain
Copy link
Contributor Author

mikesplain commented Aug 24, 2017

Sorry for the delay, version bumped @lachie83 @unguiculus

@lachie83
Copy link
Contributor

lachie83 commented Aug 24, 2017

@mikesplain we need to bump again. The rebase nullified your change.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 25, 2017
@mikesplain
Copy link
Contributor Author

mikesplain commented Aug 25, 2017

@lachie83 Ugh, sorry about that. Had a bad git day yesterday.... Fixed now.

@lachie83 lachie83 added code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed labels Aug 25, 2017
@lachie83
Copy link
Contributor

lachie83 commented Aug 25, 2017

LGTM

@lachie83 lachie83 merged commit ca92186 into helm:master Aug 25, 2017
lachie83 added a commit to lachie83/charts that referenced this pull request Aug 29, 2017
@lachie83
Merge remote-tracking branch 'upstream/master'
b7d1ab1 
* upstream/master: (272 commits)
  [incubator/redis-cache] helm#1785 namespace defined templates with chart name (helm#1840)
  [incubator/etcd] helm#1785 namespace defined templates with chart name (helm#1835)
  Correct Uninstall Instructions (helm#1831)
  [incubator/patroni] helm#1785 namespace defined templates with chart name (helm#1832)
  [incubator/kube-registry-proxy] helm#1785 namespace defined templates with chart name (helm#1833)
  [incubator/docker-registry] helm#1785 namespace defined templates with chart name (helm#1836)
  [incubator/check-mk] helm#1785 namespace defined templates with chart name (helm#1837)
  [incubator/tensorflow-inception] helm#1785 namespace defined templates with chart name (helm#1838)
  [incubator/istio] helm#1785 namespace defined templates with chart name (helm#1839)
  [stable/etcd-operator] Add cluster.restore, cluster.pod, nodeSelector & update (helm#1200)
  Spelling and Whitespace Corrections (helm#1853)
  Added nodeselector and tolerations support for fluent-bit (helm#1846)
  [stable/traefik] Bump Traefik version to 1.3.6 (helm#1845)
  [stable/mediawiki] Release 0.4.16 (helm#1830)
  [stable/kube2iam] Add ability to configure node tolerations (helm#1829)
  Use latest kube2iam (helm#1825)
  Upgrade nginx-ingress defaultbackend image (helm#1791)
  [stable/weave-cloud] sync with upstream (helm#1804)
  [stable/fluent-bit] release 0.1.6 (Fluent Bit 0.11.17) (helm#1813)
  [stable/joomla] Release 0.4.15 (helm#1805)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@lachie83 lachie83 lachie83 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. code reviewed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. UX reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mikesplain @k8s-ci-robot @lachie83 @chancez @viglesiasce @unguiculus