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 and update cb-network APIs #1100

Merged
merged 2 commits into from
May 10, 2022

Conversation

yunkon-kim
Copy link
Member

cb-network APIs 변경사항을 반영하고, CB-Tumblebug에 신규 API를 추가하는 PR 입니다.

신규 API는 Cloud Adaptive Network에 클라우드 정보를 주입하기 위한 API 입니다.
Note - 클라우드 정보는 CB-Tumblebug TbVmInfo 객체의 CloudType, Region, Zone, VpcIID.SystemId, SubnetIID.SystemId를 활용 합니다.

변경사항 요약은 아래와 같습니다 ^^

변경사항 요약

  • Upgrade the cb-network packages to v0.0.14
  • Add a new API to inject cloud information for Cloud Adaptive Network
  • Add handler functions for the new API
  • Update to align with the cb-network v0.0.14 APIs

- Upgrade cb-network packages to v0.0.14 (CB-Larva v0.0.14)
- Add a new API to inject cloud information for Cloud Adaptive Network
- Add handler functions for the new API
- Update to align with the cb-network v0.0.14 APIs
@yunkon-kim yunkon-kim requested a review from seokho-son as a code owner May 9, 2022 12:10
@yunkon-kim yunkon-kim self-assigned this May 9, 2022
@yunkon-kim yunkon-kim requested a review from jihoon-seo as a code owner May 9, 2022 12:10
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 감사합니다. :)
마이너한 의견 남겨보았습니다. 선택적으로 반영하시면 될 것 같습니다.

// @Summary Get monitoring data of specified MCIS for specified monitoring metric (cpu, memory, disk, network)
// @Description Get monitoring data of specified MCIS for specified monitoring metric (cpu, memory, disk, network)
// @Tags [Infra service] MCIS Resource monitor (for developer)
// RestPostInjectCloudInformationForCloudAdaptiveNetwork godoc
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
// RestPostInjectCloudInformationForCloudAdaptiveNetwork godoc
// RestPutInjectCloudInformationForCloudAdaptiveNetwork godoc

Copy link
Member

@seokho-son seokho-son May 9, 2022

Choose a reason for hiding this comment

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

func GetFuncName() string {

func GetFuncName() string {
	pc := make([]uintptr, 1)
	runtime.Callers(2, pc)
	f := runtime.FuncForPC(pc[0])
	return f.Name()
}

fmt.Println(" - HTTP Status: " + strconv.Itoa(resp.StatusCode()) + " in " + GetFuncName())

		case resp.StatusCode() >= 400 || resp.StatusCode() < 200:
			fmt.Println(" - HTTP Status: " + strconv.Itoa(resp.StatusCode()) + " in " + GetFuncName())
			err := fmt.Errorf(string(resp.Body()))
			CBLog.Error(err)
			content := ConnConfig{}
			return content, err
		}

뭐.. 이런 형태로 runtime func 명칭을 불러오는 방법이 있긴 한데
코드 작성은 편해지고~
성능은 조금 떨어질 수도 있는? 코드입니다.. ㅎㅎ (크게 신경쓸 수준은 아니겠지만, 얼마나 떨어지는지는 모르겠어요 ㅎㅎ)

Copy link
Member Author

Choose a reason for hiding this comment

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

@seokho-son 주석 관련하여 수정 반영 하겠습니다.

GetFuncName() 관련 코멘트를 잘 이해하지 못하였습니다 😅 설명 부탁드리겠습니다.

Copy link
Member

Choose a reason for hiding this comment

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

@yunkon-kim GetFuncName() 을 사용하면, 로그 메시지 출력 등에서 구동 중인 함수의 명칭을 보여줍니다. 로그에 직접 해당 함수명에 대한 string을 static하게 넣지 않고, 런타임에서 dynamic하게 보여주는 방법입니당.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@seokho-son 로깅 관련 팁을 알려주신거였군요 ㅎㅎ 주석 관련한 부분으로 오해를 했는데 이제 이해했습니다 ^^

src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
src/core/mcis/network.go Show resolved Hide resolved
@yunkon-kim
Copy link
Member Author

@seokho-son

리뷰의견 및 논의사항을 반영하였습니다 ^^
(PR이 머지되면 API 업데이트가 진행될 것 같습니다.)

Comment on lines +530 to +531
VirtualNetworkID: vmObject.CspViewVmDetail.VpcIID.SystemId,
SubnetID: vmObject.CspViewVmDetail.SubnetIID.SystemId,
Copy link
Member

Choose a reason for hiding this comment

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

CB-Tumblebug TbVmInfo 객체의 필드 중 CspViewVmDetail 은 CB-Spider의 output을 저장해 놓은 값이어서
VpcIID.SystemId, SubnetIID.SystemId 는 CB-Spider가 관리하는 각 객체의 IID 중 SystemId 값이라고 보시면 되겠습니다.
TB vNet 객체에도 해당 값을 저장해 놓고 있기는 한데 (cspVNetId 필드)
TB VM 객체에서 해당 값을 읽으려면

  1. TB VM 객체의 vNetIdsubnetId string 값을 읽고
  2. 해당 vNetId 값으로 GetResource 함수를 호출해 vNet object를 얻고
  3. vNet object의 cspVNetId 필드를 보면 되기는 합니다만

비효율적이고 번거로워 보여서
지금 코드도 좋은 것 같습니다. 😊


다른 이야기인데..
위의 2번을 보다 보니
현재 GetResource 함수로는 subnet은 Get 할 수 없고
subnet 등을 Get 할 수 있는 GetChildResource 함수가 없네요..
나중에 필요하면 추가해야 할 것 같습니다. 😅

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@jihoon-seo 자세히 설명해주셔서 감사합니다 ^^

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.

lgtm

@seokho-son seokho-son merged commit fa25285 into cloud-barista:main May 10, 2022
@yunkon-kim yunkon-kim deleted the add-and-update-API branch May 11, 2022 00:45
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.

3 participants