Skip to content

Graceful exit on smf context init failure #428

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

Conversation

protohuf
Copy link
Contributor

During testing I encountered a situation where DNS lookup of the pfcp failed. This caused SMF to attempt to access a non-existent object which crashed the service (SEGFAULT).

I also noticed that InitSmfContext does return nil under some other circumstances but the return value is not checked (which would cause the service to attempt to access an uninitialized smfCtx).

So I added a check to m make sure the context is valid after init and also made DNS errors cause an invalid context state

@protohuf protohuf force-pushed the graceful_exit_on_smf_context_init_failure branch from 0430814 to d17fa0e Compare June 16, 2025 10:37
@gab-arrobo gab-arrobo requested a review from Copilot June 19, 2025 04:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the SMF service's resilience during initialization by ensuring that an unsuccessful SMF context initialization is detected early, preventing segfaults. Key changes include:

  • Adding a nil-check after SMF context initialization in service/init.go to exit gracefully if initialization fails.
  • Updating Docker and related dependencies in go.mod.
  • Updating the PFCP address resolution error handling in context/context.go to return nil on failure.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
service/init.go Adds a nil-check for the SMF context and logs a fatal error if initialization fails.
go.mod Updates Docker dependency version and removes deprecated dependencies.
context/context.go Returns nil when PFCP address resolution fails, marking invalid context initialization.
Comments suppressed due to low confidence (1)

go.mod:41

  • [nitpick] It may be beneficial to add a comment or update the commit message to document why the Docker dependency was updated to v25.0.6. This context can help future maintainers understand the rationale behind the dependency changes.
	github.com/docker/docker v25.0.6+incompatible // indirect

@gab-arrobo gab-arrobo requested a review from a team June 20, 2025 01:17
Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

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

+1

@gab-arrobo
Copy link
Contributor

@thakurajayL, @ghislainbourgeois, what are your thoughts?

@gab-arrobo
Copy link
Contributor

Hi @protohuf,
Quick question... How can we replicate this issue you are specifically encountered? I am asking this because I was able to replicate the DNS lookup error by setting the pfcp.addr to some non-existing name/address. That is, how are you deploying the smf?

net.DNSError {UnwrapErr: error nil, Err: "server misbehaving", Name: "smf"...}

@protohuf
Copy link
Contributor Author

Hi @gab-arrobo The short version of the story is that we (our organisation) is experimenting and examining the various SD-Core components in order to get a deeper understanding of the project. This particular issue arose as we where individually setting up each component without any containerization (just running the services directly on bare metal).

So there are no direct deployment method that's currently causing this to happen. I just think it's better if a process dies gracefully rather than segfaulting :)

Copy link
Contributor

@ghislainbourgeois ghislainbourgeois left a comment

Choose a reason for hiding this comment

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

+1

I think with the log level change suggested by @gab-arrobo this should be good to merge.

Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

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

+1

@gab-arrobo gab-arrobo merged commit 2d974f6 into omec-project:main Jul 1, 2025
8 checks passed
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