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

[ws-daemon] Add ws-daemon headless service #3473

Merged
merged 2 commits into from
Jun 25, 2021

Conversation

rl-gitpod
Copy link
Contributor

@rl-gitpod rl-gitpod commented Mar 17, 2021

Use endpoint on the ws-manager node to avoid pod/nodePort access issues.
Addresses #2956

@rl-gitpod rl-gitpod force-pushed the rl/2956-no-ws-daemon-nodeport branch from 7080be3 to 5966598 Compare March 19, 2021 02:49
@rl-gitpod rl-gitpod requested a review from csweichel March 19, 2021 03:00
@geropl
Copy link
Member

geropl commented Mar 19, 2021

Thx for your contribution @rl-gitpod !

Approach and implementation look good to me. @csweichel ?

@aledbf
Copy link
Member

aledbf commented Mar 21, 2021

@rl-gitpod, why not just replace/improve/fix the current approach? Adding an option without any additional check for the CNI will lead to the same restriction and problems.

@rl-gitpod
Copy link
Contributor Author

@rl-gitpod, why not just replace/improve/fix the current approach? Adding an option without any additional check for the CNI will lead to the same restriction and problems.

@aledbf - not sure I follow.
This replaces/fixes the current approach for ws-manager accessing ws-daemon as per #2956

I agree that there is a much bigger and potentially more impactful aspect of using the nodePort and the existing wso.HostIP() elsewhere throughout the code base - which is perhaps what you are referring to? If so, that was the thinking but beyond the scope of this issue and I'm unsure of where that fits in the short/medium term.

@stale
Copy link

stale bot commented Jun 19, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jun 19, 2021
@aledbf
Copy link
Member

aledbf commented Jun 19, 2021

/werft run

👍 started the job as gitpod-build-rl-2956-no-ws-daemon-nodeport.2

@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jun 19, 2021
@aledbf
Copy link
Member

aledbf commented Jun 19, 2021

@csweichel friendly ping

@csweichel
Copy link
Contributor

csweichel commented Jun 21, 2021

/werft run

👍 started the job as gitpod-build-rl-2956-no-ws-daemon-nodeport.3

Use endpoint on the ws-manager node to avoid pod/nodePort access issues.
@csweichel csweichel force-pushed the rl/2956-no-ws-daemon-nodeport branch from 5966598 to b8a63c2 Compare June 21, 2021 08:51
@csweichel
Copy link
Contributor

csweichel commented Jun 23, 2021

/werft run

👍 started the job as gitpod-build-rl-2956-no-ws-daemon-nodeport.8

@csweichel csweichel force-pushed the rl/2956-no-ws-daemon-nodeport branch from 1b56704 to 40ec397 Compare June 23, 2021 07:20
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #3473 (40ec397) into main (2f7cc15) will increase coverage by 37.07%.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff            @@
##           main    #3473       +/-   ##
=========================================
+ Coverage      0   37.07%   +37.07%     
=========================================
  Files         0       14       +14     
  Lines         0     3903     +3903     
=========================================
+ Hits          0     1447     +1447     
- Misses        0     2337     +2337     
- Partials      0      119      +119     
Flag Coverage Δ
components-ws-manager-app 37.07% <84.84%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/ws-manager/pkg/manager/annotations.go 66.66% <ø> (ø)
components/ws-manager/pkg/manager/monitor.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/manager.go 27.39% <88.88%> (ø)
components/ws-manager/pkg/manager/status.go 72.32% <100.00%> (ø)
...omponents/ws-manager/pkg/manager/pod_controller.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/headless.go 44.00% <0.00%> (ø)
components/ws-manager/pkg/manager/imagespec.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/probe.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/create.go 79.40% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f7cc15...40ec397. Read the comment docs.

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

LGTM

@csweichel csweichel merged commit eb0f0c3 into main Jun 25, 2021
@csweichel csweichel deleted the rl/2956-no-ws-daemon-nodeport branch June 25, 2021 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants