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

Support upgradecluster API and update test scripts for cluster #1462

Merged

Conversation

sykim-etri
Copy link
Member

@sykim-etri sykim-etri commented Mar 18, 2024

K8s 클러스터 버전을 업그레이드할 수 있는 UpgradeCluster API를 추가하고 클러스터 API를 시험하기 위한 테스트 스크립트를 업데이트하였습니다.
테스트 스크립트와 관련하여서는 개별 클러스터 API 시험 스크립트 내 CSP에 의존적인 코드를 제거하고 conf.env를 활용하도록 수정하였습니다.

@github-actions github-actions bot added the docs Improvements or additions to documentation label Mar 18, 2024
@sykim-etri sykim-etri changed the title Update test scripts for cluster Support UpgradeCluster API and update test scripts for cluster Mar 18, 2024
@sykim-etri sykim-etri changed the title Support UpgradeCluster API and update test scripts for cluster Support upgradecluster API and update test scripts for cluster Mar 18, 2024
@sykim-etri sykim-etri force-pushed the update-test-scripts-for-cluster branch 2 times, most recently from fc86065 to 6ef90a6 Compare March 19, 2024 04:51
@sykim-etri sykim-etri added the api PR related with api label Mar 19, 2024
@sykim-etri sykim-etri force-pushed the update-test-scripts-for-cluster branch 6 times, most recently from dcaf631 to d3a8bb4 Compare March 20, 2024 12:31
Copy link
Member

@seokho-son seokho-son left a comment

Choose a reason for hiding this comment

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

@sykim-etri 기여 감사합니다!
간단한 질문이 있어서 남겨두었습니다.

테스트 스크립트는 꼼꼼히 살펴보지는 않았으나, 개선이 필요하다면 향후PR 머지 후에 진행해도 좋을 것 같습니다.

* Build RequestBody for SpiderUpgradeClusterReq{}
*/
requestBody := SpiderUpgradeClusterReq{
NameSpace: "", // should be empty string from Tumblebug
Copy link
Member

Choose a reason for hiding this comment

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

  • 여기서 NameSpace 는 무엇을 의미하는지요?
  • 요청시 해당 파라미터를 제공하지 않으면, 문제가 생기나요? 필요없다면 제외해도 될 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 필드는 CB-SP에서 클러스터 제어시 CB-TB에 의한 요청인 경우와 그렇지 않은 경우 유사 형식 이름(namespace+name 형식)을 갖도록 처리하기 위한 항목으로 파악하고 있습니다.
CB-SP에서 확인하는 필드이기 때문에 현재로는 필수 항목으로 알고 있습니다.
@powerkimhub

Copy link
Member

Choose a reason for hiding this comment

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

@sykim-etri 추가적으로 질문드립니다.

  • func UpgradeCluster 는 기존 k8s의 클러스터의 버전을 변경하는 기능으로 이해했습니다. 이 경우, 유사 형식 이름(namespace+name 형식)을 갖도록 처리하는 것이 어떠한 경우에 필요한지 이해가 어렵습니다. SP가 인지하는 클러스터의 ID(구분자)만 내려보내면 되는 것은 아닌지요?

  • 만약 꼭 필요한 정보라고 하더라도, TB의 namespace에 해당하는 값을 전달해줘야 하는 것은 아닌가요?

상기 질문은, SP에 요청하기 위한 필드인 namespace가 SP에서의 필수 사항이라고 하니, 본 PR의 승인 여부와는 무관합니다. :)

Copy link
Member

Choose a reason for hiding this comment

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

@sykim-etri @seokho-son

  • Tumblebug에서 호출할때는 사용 안하셔도 될거에요.
    • 명시적으로 설정 안하시면, Namespace=""와 같을 거고,
    • 그러면, Spider에서는 Namespace 관련해서 아무것도 안합니다.
  • Spider에서 Namespace를 사용하는 경우는
    • WebTool(WT)에서 직접 TB 통하지 않고 직접 Cluster 제어를 요청할때 입니다.
    • 이 경우에는 WT이 Cluster에 사용할려고 TB에 VNet 등의 자원을 생성할 때 사용한 Namespace를
    • Spider에 내려 보내는 용도로 활용되었습니다.
    • TB 통합이 완료 되면 사용이 안될 것입니다.

[참고]

Copy link
Member Author

Choose a reason for hiding this comment

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

  • 만약 꼭 필요한 정보라고 하더라도, TB의 namespace에 해당하는 값을 전달해줘야 하는 것은 아닌가요?

TB에서 SP로 요청시 자원 이름을 nsId + resourceId 형식으로 보내고 있기 때문에, 클러스터의 경우도 이를 고려하여 사용자가 직접 SP에 요청하는 경우 NameSpace를 입력할 수 있도록하여 타 자원과 유사한 이름을 유지할 수 있도록 지원하기 위함으로 알고 있습니다.

  • func UpgradeCluster 는 기존 k8s의 클러스터의 버전을 변경하는 기능으로 이해했습니다. 이 경우, 유사 형식 이름(namespace+name 형식)을 갖도록 처리하는 것이 어떠한 경우에 필요한지 이해가 어렵습니다. SP가 인지하는 클러스터의 ID(구분자)만 내려보내면 되는 것은 아닌지요?

CreateCluster도 동일하게 NameSpace 필드가 존재하기 때문에 UpgradeCluster도 동일하게 NameSpace 필드가 존재하게 된 것입니다. TB 지원이 없었을 당시 설계된 필드이므로 지금은 불필요할 수도 있겠습니다.

@powerkimhub

Copy link
Member

Choose a reason for hiding this comment

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

@sykim-etri 상세한 설명 감사합니다. 향후 개선 포인트로 생각하고 있겠습니다. :)

Comment on lines 191 to 215
type SpiderUpgradeClusterReq struct {
NameSpace string // should be empty string from Tumblebug
ConnectionName string
ReqInfo SpiderUpgradeClusterReqInfo
}

type SpiderUpgradeClusterReqInfo struct {
Version string
}

type TbUpgradeClusterReq struct {
Version string `json:"version"`
}
Copy link
Member

Choose a reason for hiding this comment

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

public struct 에 대한 주석이 누락되어 있습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

보완하였습니다.

@@ -1537,6 +1551,160 @@ func DeleteAllCluster(nsId string, subString string, forceFlag string) (common.I
return deletedClusters, nil
}

// UpgradeCluster upgrades the TB Cluster object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// UpgradeCluster upgrades the TB Cluster object
// UpgradeCluster upgrades an existing cluster to the specified version

조금 더 TB 시스템 관점에서 명확하게 주석을 남기면 좋을 것 같습니다. (클러스터 오브젝트 자체만 업그레이드 하는 것이 아니라, 실제 버전을 업그레이드 하는 것이므로)

Copy link
Member Author

Choose a reason for hiding this comment

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

보완하였습니다.

@sykim-etri sykim-etri force-pushed the update-test-scripts-for-cluster branch from d3a8bb4 to d69f6aa Compare March 21, 2024 08:55
@seokho-son
Copy link
Member

/lgtm

@github-actions github-actions bot added the lgtm This PR is acceptable by at least one reviewer label Mar 21, 2024
@seokho-son
Copy link
Member

/approve

@github-actions github-actions bot added the approved This PR is approved and will be merged soon. label Mar 21, 2024
@cb-github-robot cb-github-robot merged commit 9dc4500 into cloud-barista:main Mar 21, 2024
5 checks passed
@sykim-etri sykim-etri deleted the update-test-scripts-for-cluster branch June 26, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api PR related with api approved This PR is approved and will be merged soon. docs Improvements or additions to documentation lgtm This PR is acceptable by at least one reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants