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(k8s): add WaitForPool & WaitForClusterPools helper methods #312

Merged
merged 1 commit into from
Feb 18, 2020

Conversation

debovema
Copy link
Contributor

@debovema debovema commented Feb 7, 2020

Copy link
Contributor

@Sh4d1 Sh4d1 left a comment

Choose a reason for hiding this comment

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

Okay, so I have a different approach:

  • WaitForNodesReadyRequest with either a ClusterID or a PoolID and a Region
  • WaitForNodesReady which will:
    • check the status of the pool and the targetsize == current node count if poolID is not empty
    • chech the status of each pool and current node count of cluster == sum of pool target size is clusterID is not empty
    • fail if both are empty

@QuentinBrosse @jerome-quere any thoughts?

api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@QuentinBrosse QuentinBrosse left a comment

Choose a reason for hiding this comment

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

@Sh4d1

* `WaitForNodesReadyRequest` with either a `ClusterID` or a `PoolID` and a `Region`

I prefer 2 methods (eg. WaitForCluserNodesReady and WaitForPoolNodesReady)

api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
@debovema
Copy link
Contributor Author

@Sh4d1 @QuentinBrosse : thanks for your comments. I will try to have a look asap.

@debovema
Copy link
Contributor Author

WaitForCluserNodesReady

just to be sure, WaitForClusterNodesReadywould just call WaitForPoolNodesReady for all the pools of the cluster ?

@Sh4d1
Copy link
Contributor

Sh4d1 commented Feb 11, 2020

@debovema Since the implementation of pool ready will be to check if target size == current_node_count and the status == ready, I guess you can

  • either loop through each pool and call this method
  • or check that each pool is ready and current_node_counf of cluster == sum of target size for each pool

I think both implementation are quite the same in term of performance. Maybe opt for the first one since you can in fact reuse the WaitForPoolNodesReady :)

@debovema
Copy link
Contributor Author

  • WaitForNodesReadyRequest with either a ClusterID or a PoolID and a Region

For this one, I am not yet as familiar as you with the Scaleway API but why do we need to provide a PoolID and a Region in the request ? It means that a PoolID is not unique across region right ? But what if we provide directly a *k8s.Pool parameter in the request ?

@Sh4d1
Copy link
Contributor

Sh4d1 commented Feb 11, 2020

This method won't be implemented no?
I guess the cleaner way is to pass the clusterID to WaitForClusterNodesReady and a poolID to WaitForPoolNodesReady. (Since you only need the ID anyway I think)

@debovema debovema changed the title feat(k8s): add WaitForPoolNodes helper method feat(k8s): add WaitForPoolNodesReady helper method Feb 11, 2020
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
@debovema debovema force-pushed the wait-for-nodes branch 3 times, most recently from 4932a2d to 676bae7 Compare February 13, 2020 20:30
@debovema debovema force-pushed the wait-for-nodes branch 4 times, most recently from dfd9f6c to eee3cd4 Compare February 13, 2020 20:39
@debovema debovema changed the title feat(k8s): add WaitForPoolNodesReady helper method feat(k8s): add WaitForPoolNodesReady & WaitForClusterNodesReady helper methods Feb 13, 2020
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
api/k8s/v1beta4/k8s_helpers.go Outdated Show resolved Hide resolved
@debovema debovema force-pushed the wait-for-nodes branch 2 times, most recently from 5913497 to 34b1b50 Compare February 13, 2020 21:03
@debovema debovema marked this pull request as ready for review February 13, 2020 21:09
@debovema debovema force-pushed the wait-for-nodes branch 3 times, most recently from dc79d73 to c74a509 Compare February 14, 2020 13:29
@debovema debovema force-pushed the wait-for-nodes branch 3 times, most recently from fc9f601 to 334b376 Compare February 18, 2020 12:17
@debovema debovema changed the title feat(k8s): add WaitForPoolNodesReady & WaitForClusterNodesReady helper methods feat(k8s): add WaitForPool & WaitForClusterPools helper methods Feb 18, 2020
@debovema debovema force-pushed the wait-for-nodes branch 2 times, most recently from f44c438 to 02a4f06 Compare February 18, 2020 12:31
Sh4d1
Sh4d1 previously approved these changes Feb 18, 2020
@Sh4d1
Copy link
Contributor

Sh4d1 commented Feb 18, 2020

LGTM

Copy link
Contributor

@loicbourgois loicbourgois left a comment

Choose a reason for hiding this comment

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

LGTM

@loicbourgois loicbourgois merged commit d26e180 into scaleway:master Feb 18, 2020
@debovema debovema deleted the wait-for-nodes branch February 20, 2020 10:43
@remyleone remyleone added the k8s Kubernetes Kapsule issues, bugs and feature requests label Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k8s Kubernetes Kapsule issues, bugs and feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants