-
Notifications
You must be signed in to change notification settings - Fork 40k
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
downward api volume plugin #5093
Conversation
@thockin loves sidecars. |
} | ||
|
||
// generateTmpFile writes input parameter 'values' in a temporary file. File descriptor | ||
// is returned. In case error is returned file descriptor is not meaningfull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: typo: meaningful
I do like this! Is this something we think we want to pursue? It seems viable to me... |
@thockin If it does the job, I think we can pursue it. One thing that has come up in discussion is concern about the load on the api server as you scale up usage of this sidecar in pods. There's been some concern over making the pod have to use a sidecar container for this in environments where every pod needs to consume this info. @sdminonne @EricMountain-1A do you want to add to that at all? All that said, this seems like what we will want to go with if we want to move toward all user concerns that use resources happening in a user container. Either way you will have the same amount of disk IO, and if you make the platform provide some baked-in first-class support for this the complexity of the API required to express it starts to approach the API required to fully express a container (as you've mentioned before). I believe the only thing that would change is that you'd have one pod watch per node (once #4777 is implemented) instead of an additional watch per pod. Any thoughts, @smarterclayton? |
I've a good discussion with @smarterclayton who clarified me some point about re-labeling process (more specifically the fact that a pod is going to be restarted automatically) and the expected load on the master. The latter was my major concern. As soon the load on the master is under control I'm ok with this mechanism (which I finalize anyway) and we may use it. |
Assuming this uses watch properly, how would the load on the API server be If that is an issue, I bet we could do something like watch the kubelet's On Mon, Mar 9, 2015 at 10:58 AM, Salvatore Dario Minonne <
|
Since the kubelet has the labels, and already watches, why can't the pod talk to the kubelet to get this, maybe via the metadata service thing that Rocket has proposed. ----- Original Message -----
|
I think that is more or less what I said :) If you squint. On Tue, Mar 10, 2015 at 8:25 AM, Clayton Coleman notifications@github.com
|
You've proven I actually don't read what anyone else writes. /hangs-head
|
How is the name of the pod supposed to get set? This seems pretty heavyweight and opaque to me. |
@thockin @smarterclayton Yep, I had had pod querying kubelet and a single watch from kubelet in mind. @sdminonne Any opinions on that? |
@thockin, @smarterclayton @pmorie sorry for the dealy on this. Clues? |
Kubelet already has watch
|
Kubelet's API should be a reflection of the API server for things that the On Thu, Mar 12, 2015 at 7:49 AM, Clayton Coleman notifications@github.com
|
@smarterclayton @thockin the problem I'm facing is the way to get the pod name. For the kubelet I can watch or poll |
Yeah, we've been punting on exposing that information - Clayton doesn't On Mon, Mar 16, 2015 at 9:30 AM, Salvatore Dario Minonne <
|
I'll make it a priority to iterate the current pull request proposal. The upstream change to Docker to resolve env automatically was the latest wrinkle
|
78547ff
to
d017361
Compare
} | ||
} | ||
if strings.HasPrefix(items[0], "..") && len(items[0]) > 2 { | ||
allErrs = append(allErrs, errs.NewFieldForbidden("path", "must not start with \"..\".")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be Invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done
Sorry, a bit of imprecision on the last go-around. |
GCE e2e build/test failed for commit b6255d6503c8e3c7e5f8f3a858bf41014908eb62. |
b6255d6
to
fc73855
Compare
@thockin many apologizes for the misunderstanding. |
GCE e2e build/test passed for commit fc738559c9560c5e4f11f06ae9863c63227fd486. |
|
||
## Example | ||
|
||
This is an example of a pod that consumes its labels and annotations via the downward API volume, labels and annotations ared dumped rispectively in `/etc/podlabels` and in `/etc/annotations` files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are dumped in `/etc/podlabels` and `/etc/annotations`, respectively:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmorie thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and squashed
fc73855
to
0eae3e3
Compare
GCE e2e build/test failed for commit 0eae3e364addf8db810b94ad86373b1189d0ee8f. |
the fail is really strange just modified the docs... :) |
0eae3e3
to
f4dc065
Compare
GCE e2e build/test passed for commit f4dc065. |
L G T M |
@thockin thanks :) |
@sdminonne o/\o ✊ |
@pmorie thanks to you (obviously) too |
@sdminonne Sorry this has to be rebased... I promise we will then immediately merge... |
nm, it's generated code errors, I'll do it manually. |
@brendandburns many thanks! |
This PR implements #560 especially what is discussed here
Dumping info in files is were discussed in #386 as well.
Needs rebasing after #8788 is merged