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

refactor: introduce UPNode as common interface for UPF and gNB nodes #120

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

LaumiH
Copy link

@LaumiH LaumiH commented Sep 10, 2024

I worked on refactoring the SMF code, and this is a change I found very helpful to improve code reuse and readability.

For simpler management of UPNodes in the UserPlaneTopology, a new struct is added:

type UPNode struct {
	Name   string
	Type   UPNodeType
	ID     uuid.UUID
	NodeID pfcpType.NodeID
	Links  UPPath
}

The gNB and UPF structs "inherit" these fields using Golang's struct embedding (similar to inheritance).
The *UPNode pointer is given to the constructors of gNB and UPF structs (e.g., the NewUPF method).

E.g.,

type GNB struct {
	UPNode
	ANIP net.IP
}

and

type UPF struct {
	*UPNode
...
}

Additionally, both gNB and UPF structs implement the UPNodeInterface:

type UPNodeInterface interface {
	String() string
	GetName() string
	GetID() uuid.UUID
	GetType() UPNodeType
	GetLinks() UPPath
	AddLink(link UPNodeInterface) bool
	RemoveLink(link UPNodeInterface) bool
	GetNodeID() pfcpType.NodeID
	GetNodeIDString() string
}

This makes the UUID of each UPF a uuid.UUID instead of a string.

Now, the UPPath consists of UPNodes that implement the interface:

type UPPath []UPNodeInterface

This makes the code more concise and removes unnecessary empty fields from objects.

Additional smaller changes:

  • The MatchedSelection method is moved to the upf.go file for clarity.
  • Add method configToNodeIDto convert the single NodeID string from the SMF config to either IPv4, IPv6, or FQDN NodeID type.
  • Use existing LinksFromConfiguration method in NewUserPlaneInformation method instead of code duplication.
  • The UPF UUID is now a uuid.UUID and not a string.
  • Reformat UPFSelectionParams.String() output for better readability.
  • Added a bit of logging here and there, mainly traces.
  • Replace every occurence of UPF.NodeID.ResolveNodeIdToIp().String() and similar with a method.

@andy89923 andy89923 self-requested a review September 11, 2024 05:17
@andy89923
Copy link
Contributor

Hi @LaumiH,

Do you test the functionalities with this PR? And which scenarios did you take?
I'm going to fix the linter and test this PR, and I'm wondering about the progress you made.
Thanks!

@LaumiH
Copy link
Author

LaumiH commented Sep 17, 2024

Hi @andy89923, I tested only session establishment with 1 gNB and 1 UE so far. In addition, I have tested the heartbeat procedure and UPF failure detection. I have also checked the unit tests.

Are there any test scenarios/cases that you typically do? I can help with testing.

Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

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

I haven't reviewed all the details, but here are some suggestions.

Also, I think we should test with ULCL scnario for more than 1 GNB and 1 UPF as UPNode in SMF.

internal/context/user_plane_information.go Outdated Show resolved Hide resolved
internal/context/user_plane_information.go Outdated Show resolved Hide resolved
internal/context/user_plane_information.go Show resolved Hide resolved
pkg/factory/config.go Outdated Show resolved Hide resolved
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.

2 participants