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

Add update cluster status wftpl #22

Merged
merged 3 commits into from
Nov 3, 2021
Merged

Conversation

zugwan
Copy link
Contributor

@zugwan zugwan commented Oct 13, 2021

사용자 클러스터 상태 변경 템플릿을 추가하고 클러스터 생성 마지막 단계에서 이를 호출하도록 수정하였습니다. 추가로 update-tks-info 템플릿 이름을 App 대상임을 명확히 하도록 update-tks-app-info로 변경하였습니다.

아래는 테스트 결과입니다.

Screen Shot 2021-10-13 at 21 06 39

w

다음은 고려만 하다가 적용은 되지 않은 사항입니다.

  • 클러스터 상태를 RUNNING으로 변경하기 전 확인하는 단계를 워크플로우 템플릿에 별도로 추가할 것인지, 추가한다면 무엇을 더 추가할 것인지(현재는 cluster-api-aws 차트에서 대부분 수행하고 있음)
  • 추가된 워크플로우 템플릿 안에 하나의 템플릿만 존재하는 형태인데 update-tks-app-info 처럼 update-tks-cluster-info로 템플릿 이름을 사용하고 하위에 다른 템플릿을 추가하는 라이브러리처럼 사용할 것인지: 이 이유로 update-tks-info 템플릿 이름을 바꾼 것인데 cluster info 관련 템플릿이 상태 변경외에 다른 것이 필요할 지 모호해서 일단 현재 이름으로 사용했습니다.

@@ -1,7 +1,7 @@
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
name: update-tks-info
name: update-tks-app-info
Copy link
Contributor

Choose a reason for hiding this comment

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

file이름과 template name이 일치하지 않습니다.

PR설명에 적은

추가된 워크플로우 템플릿 안에 하나의 템플릿만 존재하는 형태인데 update-tks-app-info 처럼 update-tks-cluster-info로 템플릿 이름을 사용하고 하위에 다른 템플릿을 추가하는 라이브러리처럼 사용할 것인지: 이 이유로 update-tks-info 템플릿 이름을 바꾼 것인데 cluster info 관련 템플릿이 상태 변경외에 다른 것이 필요할 지 모호해서 일단 현재 이름으로 사용했습니다.

부분과 관련있는건가요?

@ktkfree
Copy link
Contributor

ktkfree commented Oct 15, 2021

cluster 상태 관리에 대해 근본적인 물음이 계속 있습니다...

현재까지의 논의는 비동기로 워크플로를 호출하고 (INSTALLING), 본 PR 의 워크플로에서 상태를 (RUNNING) 변경하는것인데요.
만약 이 워크플로가 호출되지 않는다면(실패한다면), cluster 의 상태는 영원히 INSTALLING 상태로 남아 DB 와 데이터 동기화가 깨질수 있을 것 같아요. 또한 FAIL 의 정의도 없는 상태이고요.

관련하여 차주 오프라인 미팅에서 잠시 논의했으면 좋겠습니다~

@ktkfree ktkfree closed this Oct 15, 2021
@ktkfree ktkfree reopened this Oct 15, 2021
@ktkfree
Copy link
Contributor

ktkfree commented Oct 15, 2021

어라.. 잘못 눌러서 강제 closed 시켜버렸네요. reopen 하였습니다~

@robertchoi80
Copy link
Contributor

robertchoi80 commented Oct 18, 2021

update-app-info 와 update-cluster-status 별도의 workflow로 두지 말고, 'update-tks-info'라는 하나의 workflow 내에서 별도의 template 으로 구분하면 되지 않을까요? parameter도 정말 common한 내용만 workflow param으로 두고, 각 template에 필요한 param들은 template scope의 input param으로 변경하고, entrypoint는 아예 삭제해버리면 될것 같습니다. 말씀하신 대로 추가될 function이 많지 않아 'update-tks-info'를 하나의 library처럼 가져가는 것도 괜찮을 것 같아서요.

@intelliguy
Copy link
Contributor

지난 워크샵에서 구조를 바꾼다고 했던거 같은데 완료되서 리뷰해야하면 알려주세요

@zugwan zugwan force-pushed the add_update_cluster_status_wftpl branch from e910f6d to c519a1d Compare October 28, 2021 06:14
@zugwan
Copy link
Contributor Author

zugwan commented Oct 28, 2021

지난 논의된 결과대로 tks_info 디렉토리에 하나의 파일로 존재하던 AppGroupInfo 업데이트/삭제하는 템플릿을 각각 독립된 파일로 분리하였습니다. ClusterStatus 업데이트 하는 템플릿을 추가하고 클러스터 생성 마지막 단계에서 호출하도록 수정하였습니다.

arguments:
parameters:
- name: cluster_status
value: "{{workflow.parameters.cluster_status}}"
Copy link
Contributor

@ktkfree ktkfree Nov 2, 2021

Choose a reason for hiding this comment

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

runUpdateClusterStatus 는 어떤 용도로 있는 템플릿인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updateClusterStatus 템플릿이 외부에서 호출되어야 하고 이를 위해 cluster_status 파라미터가 존재합니다. 이 부분을 그대로 흉내내고 테스트 하기 위한 용도라고 보시면 됩니다.

@ktkfree
Copy link
Contributor

ktkfree commented Nov 2, 2021

AppGroup 설치에 대한 상태관리도 추후 필요할 것 같습니다.
이 PR 을 참고하여 동일하게 개발되면 될 것 같아요~

@zugwan zugwan merged commit 7f516c0 into main Nov 3, 2021
@zugwan zugwan deleted the add_update_cluster_status_wftpl branch November 3, 2021 02:24
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.

5 participants