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

Enhance network features and align with Spider APIs #1720

Merged
merged 12 commits into from
Sep 2, 2024

Conversation

yunkon-kim
Copy link
Member

@yunkon-kim yunkon-kim commented Aug 14, 2024

This PR will enhance network features and align with Spider APIs.

The APIs

image

You can find progress and changes in comments/commits.

Test result
You can find the test result in this discussion #1783

Info/warning - I tested this on AWS, GCP, and Azure. Some issues may remain still. If you encounter any issues on vNet/subnet, please mention @yunkon-kim to address them ASAP.

Ref: #1710

@yunkon-kim
Copy link
Member Author

@seokho-son @sykim-etri @innodreamer

Network feature 개선 작업을 추진하면서, 영향이 갈 수 있는 부분들을 중간 중간 공유 드리려고 합니다.

우선, Create vNet API와 Register vNet API의 분리 계획을 공유 드립니다. 참고하시기 바랍니다.

  • 두 기능의 목적이 다름에도, 한 API에 query param으로 조건을 체크하도록 되어있습니다.
  • 이 참에 목적에 맞게 두 API로 분리할 계획 입니다.

@seokho-son
Copy link
Member

@seokho-son @sykim-etri @innodreamer

Network feature 개선 작업을 추진하면서, 영향이 갈 수 있는 부분들을 중간 중간 공유 드리려고 합니다.

우선, Create vNet API와 Register vNet API의 분리 계획을 공유 드립니다. 참고하시기 바랍니다.

  • 두 기능의 목적이 다름에도, 한 API에 query param으로 조건을 체크하도록 되어있습니다.
  • 이 참에 목적에 맞게 두 API로 분리할 계획 입니다.

동의합니다. 감사합니다. (저는 특별히 영향갈 사항 없을 것 같습니다.)

@yunkon-kim
Copy link
Member Author

개선된 API (노란색 형광펜) in 5044675
image

예정 사항

  • GetVNets, DeleteVNets 개선
  • Subnet CRD 추가 (+Zone)

검토 필요 사항

  • Update API 필요성 검토 (예, Update vNet/subnet)
  • Spider의 컨셉상 Subnet은 VPC의 child 자원인데, Tumblebug의 잔존 코드/주석에서 vNet과 subnet이 조금 다른 관점의 자원으로 관리되던 것으로 보여 검토 예정임

@seokho-son
Copy link
Member

api path 수정은 필요해보입니다. 아마 vNet/register 라고 하면 register와 리소스id랑 구분이 되지 않아서 문제가 생길걸예요.

@yunkon-kim
Copy link
Member Author

yunkon-kim commented Aug 27, 2024

코멘트 감사합니다 :-)

POST method 간에는 괜찮을 것 같다는 생각입니다. 어느 부분에서 요청 라우팅에 이슈가 있을 것으로 생각하시는지 파악이 어려운데요. 혹시, TB에서 충돌되는 url path가 있으면 공유해 주실 수 있을까요?

@seokho-son
Copy link
Member

코멘트 감사합니다 :-)

POST method 간에는 괜찮을 것 같다는 생각입니다. 어느 부분에서 요청 라우팅에 이슈가 있을 것으로 생각하시는지 파악이 어려운데요. 혹시, TB에서 충돌되는 url path가 있으면 공유해 주실 수 있을까요?

@yunkon-kim method로 구분되어 있으면 라우팅 자체에 문제가 생길 것 같지는 않네요. 그래도 아마~ rest의 컨벤션 상 해당 패턴의 path를 사용하지는 않았던 것 같습니다.
(특별히 생각나는 예시는 없지만 Post vNet/{id} 하는 경우도 생길지도 모르고 말이죠ㅎㅎ)

registerResources 이렇게 앞쪽 path를 변경하는 것도 한 방법일 것 같기도 합니다. 아무튼, 현재는 라우트 문제가 없으니 특별히 시급히 처리해야 하는 사항은 아니겠습니다. 정해지면, 다른 리소스들에 대해서도 vNet과 비슷하게 처리해야 할 것 같네요. :)

@yunkon-kim
Copy link
Member Author

@seokho-son

URI 상에 "/action" 생략하고 URI를 디자인하는 컨벤션도 있는 것으로 파악되었어서 "/register"가 되었던 상황입니다.

내부적으로 이슈가 되는 것을 보니, 개선하는 것이 좋을 것 같습니다. 다음번 수정 시 개선해 놓도록 하겠습니다.
지금 생각에는 POST /ns/:nsId/vNet?action=register이 될 가능성이 높다는 말씀드립니다.

(후보)

  • POST /ns/:nsId/vNet/register
  • POST /ns/:nsId/vNet/action/register
  • POST /ns/:nsId/vNet?action=register

(탈락)

  • POST /ns/:nsId/vNet/registeration
  • POST /ns/:nsId/vNet-regstration
  • POST /ns/:nsId/vNet-reg

@yunkon-kim
Copy link
Member Author

개선된 API (노란색 형광펜)

image

(참고)

  • Subnet 조회 API (예, GET /ns/:nsId/vNet/vNet/:vNetId/subnet) 제공에 대한 재고 필요, Spider에 Subnet 조회 API가 제공되지 않는 상태
  • Subnet은 특정 VNet과 연관성을 갖는 자원이라서, Subnet 생성 측면에서 uuid 부여 및 조회 메커니즘이 다소 복잡한 상태
  • vNet object와 subnet object가 별도 관리되지만 연관성을 갖음. 생성, 삭제 프로세스 상에서 object 간 일관성 유지 메커니즘이 다소 복잡할 수 밖에 없는 상태

Note - 복잡한 부분으로 인해 추후 기여/개선이 쉽지 않을 것으로 보임

@yunkon-kim
Copy link
Member Author

yunkon-kim commented Aug 29, 2024

Spider VPC/subnet API 관련 문의 및 답변 사항:
Ref) cloud-barista/cb-spider#1291 (comment)

참고 - 1번 항목에 대해서 지속 트래킹 필요

@yunkon-kim
Copy link
Member Author

yunkon-kim commented Aug 29, 2024

@seokho-son

새로운 vNet 및 Subnet을 CRUD하는 API set과, 외부의 vNet 및 Subnet을 CRUD하는 API set에 대하여 문의 드립니다.
: 다방면으로 살펴본 내용 중 2가지 안에 대해 의견을 듣고자 합니다.
: Multiple request/response bodies를 명시할 수 있다면 보다 간결하게 구현할 수 있는데,
: 아시는것 처럼 현재는 OAS 3.0 적용이 어렵기 때문에 구분하여 진행하는 (안) 입니다.
: (참고) 아래 내용에는 Subnet 관련 URL이 생략되어 있습니다.

(1안)
: 리소스를 내부/외부 리소스로 구분하는 (안)
: 외부 리소스에 대해서 Register/Unregister 제공할 수 있습니다.
: vNet/Subnet 이외의 자원에서도 동일 체계를 적용할 수 있을 것으로 보입니다.

# 새로운 VPC CRUD
POST   /ns/{nsId}/resource/vNet                 # 새 VPC 생성
GET    /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 조회
PUT    /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 전체 업데이트 <= 실제로 업데이트 기능이 필요할지는 미지수
PATCH  /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 부분 업데이트 <= 실제로 업데이트 기능이 필요할지는 미지수
DELETE /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 삭제
GET    /ns/{nsId}/resource/vNet                 # 새 VPC 목록 조회

# 기존 VPC CRUD
POST   /ns/{nsId}/externalResource/vNet             # 기존 VPC 등록
DELETE /ns/{nsId}/externalResource/vNet/{vnetId}    # 기존 VPC 등록 해제

(2안)
: vNet/Subnet 에 한정하여 내부/외부 리소스를 구분하는 (안)
: 외부 vNet/Subent 리소스에 대해서 Register/Unregister 제공할 수 있습니다.

# 새로운 VPC CRUD
// 1안과 동일

# 기존 VPC CRUD
POST   /ns/{nsId}/resource/externalVNet             # 기존 VPC 등록
DELETE /ns/{nsId}/resource/externalVNet/{vnetId}    # 기존 VPC 등록 해제

* Split and apply PUT method to register the existing vNet
  - Note - After upgrading OAS 3.0, creating and registering vNet APIs can be combined.
* Update APIs to get and delete the specified vNet
* Check if the requested network (vNet and subnets) are valid or not
* Reflect the Spider APIs, which are "add and remove subnets"
* Update APIs to create and delete a subnet associated with the specified vNet
* Check if the requested subnet is valid for the existing vNet
* Enhance validation mechanism
* Split API set to register/deregister vNet and subnets
* Align with Spider request/response bodies (i.e. go struct)
* Update API docs
@yunkon-kim yunkon-kim changed the title Add API drafts to enhance network features Enhance network features and align with Spider APIs Aug 29, 2024
@yunkon-kim yunkon-kim self-assigned this Aug 29, 2024
@yunkon-kim yunkon-kim marked this pull request as ready for review August 29, 2024 11:29
@yunkon-kim
Copy link
Member Author

PTAL @seokho-son (cc. @sykim-etri)

@yunkon-kim yunkon-kim added the hold Need to hold merge label Aug 30, 2024
@yunkon-kim
Copy link
Member Author

@seokho-son MapUI에서 테스트를 누락하여 잠시만 hold 하겠습니다.

@yunkon-kim
Copy link
Member Author

@seokho-son

확인해보니, MapUI 상에서 MCI 생성시 에러가 뜨네요.. 아마도 Rebase 과정에서 실수가 있었던 것 같습니다 ;;

Error from: http://cb-spider:1024/spider/vm] Status code: 500 Internal Server Error, Message: {\"message\":\"aws-ap-northeast-2, default-shared-aws-ap-northeast-2: does not exist!\"

@seokho-son
Copy link
Member

@yunkon-kim

# 새로운 VPC CRUD
POST   /ns/{nsId}/resource/vNet                 # 새 VPC 생성
GET    /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 조회
PUT    /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 전체 업데이트 <= 실제로 업데이트 기능이 필요할지는 미지수
PATCH  /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 부분 업데이트 <= 실제로 업데이트 기능이 필요할지는 미지수
DELETE /ns/{nsId}/resource/vNet/{vnetId}        # 새 VPC 삭제
GET    /ns/{nsId}/resource/vNet                 # 새 VPC 목록 조회

# 기존 VPC CRUD
POST   /ns/{nsId}/externalResource/vNet             # 기존 VPC 등록
DELETE /ns/{nsId}/externalResource/vNet/{vnetId}    # 기존 VPC 등록 해제
  • externalResource라는 표현을 신규로 도입하신 이유는, API 네이밍을 최대한 REST의 컨셉(무엇이든 리소스를 기준으로 생각)에 맞추고자 하신 것으로 보입니다.
    • 그런데, 표현상 CB-TB에서 다루는 리소스를 내부/외부를 나눌 수 있는가라는 고민은 있습니다.
    • 말하자면, 외부에 있는 리소스를 등록하는 행위(외부에 있는 리소스 등록하기)는 할 수 있지만, CB-TB에 등록된 리소스가 외부 리소스라고 칭하는 것이 바람직한가는 의문입니다. 아마도 API가 저렇게 표현되어 있어도, 내부적으로는 동일한 struct의 vNet 오브젝트가 추가될 것 같기도하고요. 어쨌든 REST의 주요 컨셉을 그대로 지켜나가긴 어려운 상황이긴 합니다.
    • 나중에는 TB로 만든 자원을 TB로 재등록하는 상황도 발생할 수 있지 않을까요?

  • 이를 고려하면, externalResource 라는 신규 표현보다는 내부/외부를 고민하지 않아도 되도록, /register/resource/vNet, /registerResource/vNet, /registerCspResource/vNet 등이 좀 더 직관적이지 않을까 싶습니다.
    • 참고: 현재 관련된 유사 API도 있는 상황입니다. (/registerCspResources : 해당 api는 특정 AZ의 리소스 타입에 해당하는 모든 리소스를 등록합니다.) 향후에는 API 네이밍을 적절히 수정할 필요도 있겠네요.
    • 참고: 기존에 제안하셨던, /ns/{nsId}/resource/vNet/register 와는 다른 패턴입니다. :)
  • 말씀해주신 바와 같이, Best는 역시, queryParam 별로 멀티(anyof, oneof, ...) 요청/응답 바디를 지정할 수 있도록 하는 것인 것 같습니다. 아쉽게도 아직 지원되지는 않지만, 언젠가는 지원할 수 있을 것이라 봅니다. 이때는 /register/resource/vNet --> /resource/vNet?option=register 로 손쉽게 전환할 수 있지 않을까.. 기대합니다.

@yunkon-kim
Copy link
Member Author

@seokho-son

의견 감사합니다. 오프라인에서 논의한 바와 같이 POST /ns/{nsId}/registerCspResource/vNet 으로 적용하도록 하겠습니다. DELETE의 경우, 의미상으로 deregister가 되어야 할 것 같지만 path를 맞추고 method로 구분한다는 생각으로 DELETE /ns/{nsId}/registerCspResource/vNet/{vNet} 진행 하겠습니다. 얼른 3.0을 쓸 수 있으면 좋겠네요 :-)

@seokho-son
Copy link
Member

DELETE /ns/{nsId}/deregisterCspResource/vNet/{vNet}

그러네요, de가 더 직관적일거 같긴한데, 일단 기여해주시는대로 따라가겠슴다!

@seokho-son
Copy link
Member

seokho-son commented Sep 2, 2024

@seokho-son

확인해보니, MapUI 상에서 MCI 생성시 에러가 뜨네요.. 아마도 Rebase 과정에서 실수가 있었던 것 같습니다 ;;

Error from: http://cb-spider:1024/spider/vm] Status code: 500 Internal Server Error, Message: {\"message\":\"aws-ap-northeast-2, default-shared-aws-ap-northeast-2: does not exist!\"

해당오류는 26d2dca ff5a402 에 의해 fix 되었습니다. :)

seokho-son and others added 2 commits September 2, 2024 15:12
* Create a label when a vNet or subnet is successfully created/registered
* Remove the label when a vNet or subnet is successfully deleted/deregistered
* Clean up vNet/subnet objects from kvstore if vNet/subnet creation fails
@yunkon-kim
Copy link
Member Author

@seokho-son

개선 및 테스트 완료하였습니다.

(개선 사항)

  • Label 추가, vNet/subnet이 정상적으로 생성/등록 되었을 때
  • Label 제거, vNet/subnet이 정상적으로 삭제/해제 되었을 때
  • Clean up vNet/subnet object, 생성/등록 과정이 실패 했을 때

(테스트)

  • vNet/Subnet 개별 API 테스트 후 Test result of the enhanced network features #1783 수정
  • 생성 과정이 실패 했을 때, vNet/subnet object 정상적으로 삭제됨 Test result of the enhanced network features #1783 하단에 반영
  • MapUI를 통한 MCI 생성 테스트
    • (Pass) 1 MCI, 1 VM in AWS ap-northeast-2
    • (Pass) 1 MCI, 1 VM in AWS ap-northeast-2, 1 VM in GCP asia-northeast3
    • (Fail) 1 MCI,1 VM in AWS ap-northeast-2, 1 VM in GCP asia-northeast3, 1 VM in Azure koreacentral
      • With the error: Failed to get Image Ubuntu22.04 from azure-koreacentral
      • AWS, GCP 테스트가 성공하는 것으로 볼 때, Azure의 실패 원인이 이번 PR과 관련 있는지를 파악해 봐야할 것 같습니다.

@yunkon-kim yunkon-kim removed the hold Need to hold merge label Sep 2, 2024
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.

@yunkon-kim API 명칭 관련 코멘트 입니다. :)

// @Param subnetId path string true "Subnet ID"
// @Success 200 {object} model.SimpleMsg
// @Failure 404 {object} model.SimpleMsg
// @Router /ns/{nsId}/externalResources/vNet/{vNetId}/subnet/{subnetId} [delete]
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
// @Router /ns/{nsId}/externalResources/vNet/{vNetId}/subnet/{subnetId} [delete]
// @Router /ns/{nsId}/deregisterCspResource/vNet/{vNetId}/subnet/{subnetId} [delete]

Copy link
Member Author

Choose a reason for hiding this comment

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

앗.. Path 수정을 누락했네요;;

// @Success 201 {object} model.TbVNetInfo
// @Failure 404 {object} model.SimpleMsg
// @Failure 500 {object} model.SimpleMsg
// @Router /ns/{nsId}/externalResources/vNet [post]
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
// @Router /ns/{nsId}/externalResources/vNet [post]
// @Router /ns/{nsId}/registerCspResource/vNet [post]

// @Success 201 {object} model.TbVNetInfo
// @Failure 404 {object} model.SimpleMsg
// @Failure 500 {object} model.SimpleMsg
// @Router /ns/{nsId}/externalResources/vNet/{vNetId} [delete]
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
// @Router /ns/{nsId}/externalResources/vNet/{vNetId} [delete]
// @Router /ns/{nsId}/deregisterCspResource/vNet/{vNetId} [delete]

// @Failure 404 {object} model.SimpleMsg
// @Failure 500 {object} model.SimpleMsg
// @Router /ns/{nsId}/externalResources/vNet/{vNetId} [delete]
func RestPostDeregisterVNet(c echo.Context) error {
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
func RestPostDeregisterVNet(c echo.Context) error {
func RestDeleteDeregisterVNet(c echo.Context) error {

Comment on lines 285 to 286
// RestPostDeregisterVNet godoc
// @ID RestPostDeregisterVNet
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
// RestPostDeregisterVNet godoc
// @ID RestPostDeregisterVNet
// RestDeleteDeregisterVNet godoc
// @ID RestDeleteDeregisterVNet

Comment on lines 470 to 471
g.POST("/:nsId/externalResources/vNet", rest_resource.RestPostRegisterVNet)
g.DELETE("/:nsId/externalResources/vNet/:vNetId", rest_resource.RestPostDeregisterVNet)
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
g.POST("/:nsId/externalResources/vNet", rest_resource.RestPostRegisterVNet)
g.DELETE("/:nsId/externalResources/vNet/:vNetId", rest_resource.RestPostDeregisterVNet)
g.POST("/:nsId/registerCspResource/vNet", rest_resource.RestPostRegisterVNet)
g.DELETE("/:nsId/deregisterCspResource/vNet/:vNetId", rest_resource.RestDeleteDeregisterVNet)

@yunkon-kim
Copy link
Member Author

@seokho-son 리뷰 코멘트 주신 부분 수정하였습니다.

Spider 0.9.2 릴리스 이후에, Azure의 get image 관련 이슈가 해소된 건을 공유 받았습니다.
Ref) cloud-barista/cb-spider#1304

Spider 최신 버전으로 빠르게 테스트 해보고 공유드리겠습니다.

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.

@yunkon-kim 마이너한 코멘트가 있습니다.

src/api/rest/server/server.go Outdated Show resolved Hide resolved
@yunkon-kim
Copy link
Member Author

@seokho-son

cb-spider:edge 이미지 (Digest 35ed10ea9d44)로 테스트 한 결과 입니다.
(Pass) 1 MCI,1 VM in AWS ap-northeast-2, 1 VM in GCP asia-northeast3, 1 VM in Azure koreacentral

@seokho-son
Copy link
Member

/approve

@github-actions github-actions bot added the approved This PR is approved and will be merged soon. label Sep 2, 2024
@cb-github-robot cb-github-robot merged commit a3e82d1 into cloud-barista:main Sep 2, 2024
4 of 5 checks passed
@yunkon-kim yunkon-kim deleted the 240805-15 branch September 2, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved and will be merged soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants