-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
typically better to rebase the PR than to do several merges, no? |
remove custom locally used values from values.yaml
subjects: | ||
- kind: ServiceAccount | ||
name: {{ template "mlbench.worker.fullname" . }}-sa | ||
--- |
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.
Each helm resource should be in a separate file, if we want to release that chart to the Helm Chart repository at some point.
See https://github.com/helm/helm/blob/master/docs/chart_best_practices/templates.md
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. Thanks.
assert len(api_response.items) == 1 | ||
ip = api_response.items[0].status.pod_ip | ||
|
||
response = requests.post("http://{ip}/api/metrics/".format(ip=ip), data=payload) |
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.
Is this blocking? Did you try it by wrapping the call with the kubernetes.stream
function?
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.
I was thinking about adding non-blocking version later. But it will not take too long. I can update this module before merge.
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.
kubernetes.stream
is used for watching api endpoints, no? What we want is more like having another thread to post
?
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.
It is intended to watch api endpoints, yes. But when I looked into the source, it looks like it can wrap any api call and it will do the call using websockets without blocking instead of waiting for a call to finish.
the client also has a (mostly undocumented) functionality for threads already, with the async=True keyword-argument for all. E.g. kubernetes-client/gen#67
Rebase is good for a clean history, though it can be more work in a long lived feature branch. From what I know it comes down to personal choice and there's not settled "better" approach. The official GitFlow docs don't have a preference either way ( https://www.atlassian.com/git/tutorials/merging-vs-rebasing ) |
@martinjaggi @Panaetius Perhaps I should not merge the updates too early. What do you suggest in this situation? |
compose/worker/Dockerfile
Outdated
ENV PYTHONPATH /codes | ||
|
||
RUN pip install kubernetes |
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 probably be done with a requirements.txt and pip install, especially if we add additional packages in the future
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.
One thing to mention is that installing kubernetes python client from anaconda will downgrade some core packages. So I use pip this time.
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.
But you can use pip with a requirements file as well, pip install -r requirements.txt
, which also let's you write what version to install. Then if kubernetes
gets updated it doesn't suddenly break the build. No need for anaconda.
No description provided.