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

feat: Allow to specify resource version for executor #4941

Closed
wants to merge 1 commit into from
Closed

feat: Allow to specify resource version for executor #4941

wants to merge 1 commit into from

Conversation

terrytangyuan
Copy link
Member

One use case for this is that we'd like to retrieve pods from cache instead of always relying on remote storage which would introduce a lot of burden on our apiserver when the scale is large.

Signed-off-by: terrytangyuan terrytangyuan@gmail.com

Checklist:

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
@alexec
Copy link
Contributor

alexec commented Jan 25, 2021

Could retrieve from chance be default?

@terrytangyuan
Copy link
Member Author

I think so but I don't want to mess up with default yet. Some applications may still rely on getting real-time updates by querying from apiserver directly. Changing default to be "0" would definitely be more scalable but the results are not 100% most recent, depending on how often the cache is updated.

@alexec
Copy link
Contributor

alexec commented Jan 25, 2021

I’m not use this will make a difference? We only call list once. I think I understand this code better now. You could completely remove the list call, and then do watch without resource version. Watches with resource version don’t work if the watch gets old, or the the resource gets old with out change. IMHO a design flaw in the API which make resource version useless. I’ve been phasing it’s usage out elsewhere in the code.

Could I ask you to remove the list and resource version code completely? I think it will have better performance AND be more robust.

@alexec
Copy link
Contributor

alexec commented Jan 25, 2021

@terrytangyuan leave this with me. I'm going to re-write the code that listens to pods as it is really complex and will be buggy.

@alexec alexec closed this Jan 25, 2021
@terrytangyuan terrytangyuan deleted the resource-version-executor branch January 25, 2021 16:54
@terrytangyuan
Copy link
Member Author

Got it. Sounds good. Thanks!

@alexec
Copy link
Contributor

alexec commented Jan 27, 2021

@terrytangyuan I've abandoned my work on this because my solution has scaling issues. Would you like to complete this PR?

#4947

@terrytangyuan
Copy link
Member Author

Do you mean completing this PR #4941 or looking into why #4947 has scaling issues?

@terrytangyuan terrytangyuan restored the resource-version-executor branch January 27, 2021 14:51
@terrytangyuan
Copy link
Member Author

I've done some experiments and it looks like it's fine to specify resource version in the watch call but maybe it's just that I haven't met a case you mentioned yet. Perhaps in order to avoid unnecessary issues I can update this PR to only specify resource version in the getPod() call. WDYT?

@alexec
Copy link
Contributor

alexec commented Jan 27, 2021

This PR. I’ve found that we establish as many pod watches as pods. This is a mistake and might explain why users are having trouble starting large workflows. This is old code. I’m still investigating, so no action from you.

It would be awesome if you fancied helping beta test the emissary executor

@terrytangyuan
Copy link
Member Author

terrytangyuan commented Jan 27, 2021

Yes we've been experiencing some scaling issues as well. I'll leave this PR closed then. Regarding emissary executor, I probably won't have time to test it very soon since we've been busy supporting k8sapi executor internally at this moment. I'll definitely check it out when I get a chance but probably not at a large scale yet.

@alexec
Copy link
Contributor

alexec commented Jan 27, 2021

As a sales job, the emissary will be as performant as docker, but as secure as k8sapi.

@alexec
Copy link
Contributor

alexec commented Jan 27, 2021

#4954

@terrytangyuan
Copy link
Member Author

As a sales job, the emissary will be as performant as docker, but as secure as k8sapi.

Got it. Will check it out.

@terrytangyuan terrytangyuan deleted the resource-version-executor branch February 21, 2021 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants