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

NFTopology and related CRDs initial commit #35

Merged
merged 6 commits into from
Jun 13, 2023
Merged

NFTopology and related CRDs initial commit #35

merged 6 commits into from
Jun 13, 2023

Conversation

s3wong
Copy link
Contributor

@s3wong s3wong commented Jun 1, 2023

No description provided.

Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

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

A few comments. The biggest thing is that I don't think this belongs in these groups, and I think we need a name for NFDeployed that follows the conventions and also better captures what the resource represents.

I think we need a separate "topology" group, and most (if not all - I haven't thought each through in detail) of these would live in there.

nf_deployments/v1alpha1/nf_deployed.go Outdated Show resolved Hide resolved
nf_deployments/v1alpha1/nf_deployed.go Outdated Show resolved Hide resolved
Copy link
Contributor

@henderiw henderiw left a comment

Choose a reason for hiding this comment

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

I believe this CR does not belong in req. We could maybe do nf_service or so?
I would put the deployed within the status of the CR

nf_requirements/v1alpha1/nf_topology_types.go Outdated Show resolved Hide resolved
nf_requirements/v1alpha1/nf_topology_types.go Outdated Show resolved Hide resolved
nf_requirements/v1alpha1/nf_topology_types.go Outdated Show resolved Hide resolved
nf_requirements/v1alpha1/nf_topology_types.go Outdated Show resolved Hide resolved
nf_deployments/v1alpha1/nf_deployed.go Outdated Show resolved Hide resolved
@s3wong
Copy link
Contributor Author

s3wong commented Jun 5, 2023

@johnbelamaric

I don't think this name follows conventions. "Deployed" implies a past action having been done. Is this
describing tings that are disovered - and thus should all be "status", or is this describing user intent, in
which case it should be "spec" as it is, but it should be a noun. How is it conceptually different than
"NFDeployment"? It seems to me it is a linkage between a cluster and a NF. Maybe the name could reflect > that?

I think there is a confusion on NFDeployed (poor name, I agree). NFDeployed here is meant as a spec to drive ESA to start monitoring what it is supposed to monitor (AMF/SMF/UPFDeployment statuses for cluster A,B,C...etc). As ESA is not undergoing much changes from seed, the input CR also isn't expected to change a lot --- it is basically the same as the old "NFDeploy" CRD that was seeded before (recall the input processing piece of ESA comes from the "deployment" entity of NFDeploy controller)

As for status of NFTopology controller, I agree, it is basically reflected by seeing which packagerevision is created for this particular topology. But I haven't defined NFTopology's statuses yet (hence not part of this CRD)

@s3wong
Copy link
Contributor Author

s3wong commented Jun 5, 2023

@henderiw

I believe this CR does not belong in req. We could maybe do nf_service or so? I would put the deployed
within the status of the CR

nf_service or as John said, topology.nephio.org should be fine. Please see my reply to John on why creating a separate "NFDeployed" --- I can change the name, but that spec is meant for ESA, not for tracking status of NFTopology

Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@nephio-prow
Copy link
Contributor

nephio-prow bot commented Jun 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nephio-prow nephio-prow bot added the approved label Jun 13, 2023
@nephio-prow nephio-prow bot merged commit 5be08f7 into nephio-project:main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants