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

Refactoring Service-Mesh workflow for tks and decapod #36

Merged
merged 2 commits into from
Mar 8, 2022
Merged

Refactoring Service-Mesh workflow for tks and decapod #36

merged 2 commits into from
Mar 8, 2022

Conversation

seungkyua
Copy link
Contributor

tks 에서 decapod-flow 의 service-mesh 설치 부분을 재활용할 수 있게 함으로써 공통 로직을 제거 했음.

@seungkyua seungkyua linked an issue Feb 16, 2022 that may be closed by this pull request
#=========================================================
# Template Definition
#=========================================================
- name: copy-eck-secret
Copy link
Contributor

@robertchoi80 robertchoi80 Feb 16, 2022

Choose a reason for hiding this comment

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

이 task를 decapod 쪽의 동일 task와 비교해보면, namespace labeling 한줄 추가로 하는 것 외에는 거의 동일한 작업이고, 대신 --kubeconfig 만 추가로 붙여주는 듯 보이는데요, 그렇다면 조건문으로 처리해서 공통 task로 구현 가능하지 않을까요? 예를 들면, kubeconfig_secret_name 이 null 이 아닐 경우에만 kubeconfig 옵션을 붙여준다던지, 아니면 코드는 그대로 두고 해당 값이 null 이면, 기본 kubeconfig를 mount해서 사용한다던지요. 근데 KUBE_CONFIG 환경변수 설정 때문에 전자는 좀 어려울 수도 있겠습니다. 암튼 꼭 이 task 외에도 전반적으로 코드 공통화가 조금 더 진행되면 좋을 것 같아 여쭤봤습니다.

Copy link
Contributor

@robertchoi80 robertchoi80 Feb 27, 2022

Choose a reason for hiding this comment

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

코멘트 내용 보충합니다. 저도 비슷한 작업을 해보니 위에 제가 제안드렸던 방법은 구현 불가능한 방법이네요. 특정 조건(예를 들면 parameter값) 에 따라 kubeconfig secret mount를 할지 말지 분기하는 것이 workflow 상에서는 불가능한 것으로 보여, 제 경우엔 아예 step을 나누고 when 절로 구분하는 방향으로 작성해봤습니다. https://github.com/robertchoi80/decapod-flow/blob/main/templates/decapod-apps/remove-lma-uniformed-wftpl.yaml

의견 부탁드리고, 위에 말씀드린 대로 코드 공통화는 확실히 더 필요할 것 같습니다. 예를 들어 PR 올리신 내용 중 현재 Argocd app 지우는 코드도 decapod 쪽은 name으로, tks 쪽은 label로 지우는 등 서로 로직이 살짝 다르기 때문에, 한곳에서만 구현하고 다른쪽은 갖다쓰도록 코드 수정이 필요할 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어차피 코드량은 when 절에 의해서 같아 보이는데요. tks-flow 의 설치 부분을 없애면 의미가 있는데 tks-flow 에 설치 flow 가 계속 있다면 오히려 각각 구분해서 놓는 것이 낫지 않을까요?

Copy link
Contributor

@robertchoi80 robertchoi80 Mar 3, 2022

Choose a reason for hiding this comment

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

지난주 싱크 때 공유드려서 이미 작업 중이실 수도 있는데, 히스토리 기록용으로 그때 보여드린 파일 링크 남겨드립니다.
https://github.com/openinfradev/decapod-flow/blob/main/templates/decapod-apps/remove-lma-uniformed-wftpl.yaml#L78-L127

@@ -4,275 +4,154 @@ metadata:
name: tks-remove-servicemesh
namespace: argo
spec:
entrypoint: delete-start
entrypoint: remove-tks-service-mesh
arguments:
parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

특별한 이슈가 없다면 삭제 인터페이스를 LMA 와 유사하게(가능하다면 동일하게) 맞춰도 괜찮을 것 같습니다.
동일한 계위의 서비스가 계속 생길텐데, 가능하다면 유사한 인터페이스를 가지는게 좋을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lma 와 좀 내용이 달라요. 일단 gateway 는 추가로 다른 namespace 에 설치될 수 있어서 해당 namespace 의 gateway 만 삭제할 수 있고, istiod 가 revision 으로 여러 버전이 설치될 수 있어서 해당 버전만 지우는 작업도 해야 합니다.
추후에 istio-operator 는 제거하고 istio helm 으로 설치하면 파라미터를 좀 더 단순하게 할 수 있을 것 같네요.

#=========================================================
- name: deploy-tks-service-mesh
steps:
- - name: create-eck-secret
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분도 삭제 쪽과 같은 의견입니다. 개별 함수 호출하는 대신 decapod 쪽 workflow 통째로 호출하는 게 좋을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

통째로 호출하는 부분으로 변경했습니다.

@seungkyua
Copy link
Contributor Author

현재까지의 코멘트 전부 반영했습니다.

kubectl --kubeconfig=/etc/kubeconfig label ns ${TARGET_NAMESPACE} taco-tls=enabled
log "INFO" "${TARGET_NAMESPACE} successfully created."
fi
# - - name: deploy-ingress-temporary
Copy link
Contributor

Choose a reason for hiding this comment

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

요기도 마찬가지로 코멘트 부분 삭제하는게 좋겠습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

삭제했습니다.

@bluejayA bluejayA merged commit 613c20e into openinfradev:main Mar 8, 2022
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.

[Dev] Service Mesh 워크플로우 변경 (tks preview출시향)
4 participants