Refactor sandbox creation function arguments

[PannagaRao]

This issue is created to perform the following

• Add a dedicated list of setters for the various pod sandbox fields and use them directly on sandbox creation
• Add a new method to convert a runtime spec to a sandbox and use it on restore.
• Enable the revive linter and set it to a minimum amount of arguments.

The modifications discussed in suggest a different approach. It is proposed to relocate internal/factory packages into oci and sandbox packages. Additionally, a new file named factory.go should be created in each package to define setters and translation functions. The objective is to establish a method for converting CRI specifications and server configurations into sandbox/container instances, and also to enable the restoration of these instances from a file.

A factory object, upon creation, should generate a sandbox object. Through the use of setters, its configurations are determined, and subsequently, the sandbox object is instantiated with the defined attributes. Once created, the sandbox object becomes immutable.

[roman-kiselenko]

@PannagaRao, are you working on this? I am up to take it.

[PannagaRao]

Hello @roman-kiselenko I'm planning to work on this in the next few days.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[xw19]

@kwilczynski @PannagaRao @saschagrunert I would like to work on this.

[kwilczynski]

@kwilczynski @PannagaRao @saschagrunert I would like to work on this.

@xw19, sounds good! You can also try to self-assign next time you want to take on something. 😄

[xw19]

@kwilczynski @saschagrunert @haircommander I believe we can break down the proposed changes into three separate tasks that can be implemented independently.

Here's a breakdown of each task:

1. Refactoring Pod Sandbox/Container Initialization with Setters:
   This task involves refactoring how pod sandboxes and containers are initialized. We propose introducing dedicated setter functions for various configuration options. This will improve code organization and maintainability.
2. Implementing OCI Spec to Sandbox/Container Conversion:
   We propose adding a new method that can convert an OCI runtime specification (OCI Spec) into a sandbox or container object. This functionality will be useful for restoring sandboxes/containers from a previous state.
3. Enforcing Code Formatting with Revive: Instead of golangci-lint, we propose trying out the revive linter.

Do you agree with this breakdown of the tasks? If so, then each task can be tackled separately, which can potentially streamline development and testing.

Please let me know your thoughts.
Thanks

[saschagrunert]

Enforcing Code Formatting with Revive: Instead of golangci-lint, we propose trying out the revive linter.

I don't think we need that, the rest looks sane to me.

[xw19]

Enforcing Code Formatting with Revive: Instead of golangci-lint, we propose trying out the revive linter.

I don't think we need that, the rest looks sane to me.

I have verified that we have revive already enabled in .golangci.yml.

[xw19]

@saschagrunert @kwilczynski @haircommander I need some clarification

Current
We have sandbox struct which is internally used and Sandbox is the interface.

To instantiate a sandbox object we use

sbox := sandboxFactory.New()
...
sbox.SetConfig(config)
...
sbox.SetNameAndID()

Later we call the InitInfraContainer
which creates new container and also the resolve file apart from other stuff.

Refactor

sbox := sandboxFactory.NewSandbox(config)

The NewSandbox will actually call SetConfig and SetNameAndID

This also means we will refactor places where we are using current three lines with a single line of code.

Is this correct assumption ? I want to deal first with how we create the sandbox. Later I will deal with converting spec to sandbox.

[haircommander]

I actually think we should take a different approach. we currently have internal/factory/sandbox which creates a translation object which eventually has fields passed to internal/lib/sandbox:New() . I think actually we should move the internal/factory/sandbox bits into internal/lib/sandbox internally, having like a internal/lib/sandbox/factory.go which has a structure that edits an internal internal/lib/sandbox:Sandbox{} object. The end goal being we don't need the function internal/lib/sandbox:New() function at all, we just return the Sandbox{} object from that structure. does that make sense?

The motivation for this approach is we shouldn't be setting the majority of the fields in a sandbox after it's created, those should be immutable. Having a group of functions that does the setting but isn't able to edit a running sandbox would better reflect what we expect. However, we don't need an intermediary structure, we can do everything inside of the Sandbox{} structure that will eventually be saved. does this make sense @saschagrunert @xw19

[xw19]

@haircommander Yes it make sense thank you for the clarification. Let me try to summarize as Story

Summary:

Currently, sandbox creation involves a factory and translation object. This story proposes refactoring the sandbox creation process to make sandbox fields immutable and directly edit the Sandbox{} object.

Acceptance Criteria:

• The internal/factory/sandbox module is moved to internal/lib/sandbox.
• A new factory.go file is created within internal/lib/sandbox.
• The New() function is removed from internal/lib/sandbox.
• Sandbox fields are set within the Sandbox{} structure itself.
• Sandbox fields are immutable after creation.
• The refactoring does not introduce any new bugs or regressions.
• Replace all the places (sandbox_run*.go) with new Sandbox initialization.

Motivation:

• To improve the immutability of sandbox objects.
• To simplify the sandbox creation process.
• To make the code more maintainable and easier to understand.

I will later try to summarise the spec to sandbox creation. Please let me know if it need any change.

[haircommander]

yes! one more acceptance criteria is way more unit testing on the sandbox creation process. there are existing unit tests in internal/factory/sandbox that should be translated and any other piecs that are moved into internal/lib/sandbox should also be tested

[xw19]

@haircommander I have gone through the code and trying to ideate a solution. So this is in a way thinking out loud.

server\sandbox_run_linux:runPodSandbox creates the sandboxFactory, CNIPluginwatcher, podContainer, container and sandbox.

This is multi-step process. I feel what we can use Builder pattern instead of a Factory pattern. (https://refactoring.guru/design-patterns/builder)

Here are is some of the code that I am thinking right now

type SandboxBuilder interface {
	//reset Resets the builder to create a new object
	reset()

	GetSandBox()

	GetConfig()

	GetName()

	GetID()

	SetConfig()

	SetNameAndID()

	SetInfraContainerID()

	SetNameSpace()
	
	SetCriSandBox()
	
	SetDNSConfig()
	
	...
}

And now inside the runPodSandbox

sbuilder.SetConfig(req.Config)
sbuilder.SetNameAndID()
...
sboxName := sbuilder.Name()

sandbox = sbuilder.GetSandbox()

Call to the sbuilder.GetSandbox resets the Sandbox.

Please share your thought on this.

[haircommander]

looks good! one question: would there be multiple builder objects per cri-o server or just one?

[xw19]

@haircommander If we use a singleton pattern for SandboxBuilder, we will only be able to create one Sandbox at a time. Therefore, this is not an ideal solution, and we should use multiple Builder objects.

[xw19]

Call to the sbuilder.GetSandbox resets the Sandbox.

I think we don't need reset. The whole process will be similar to what we do now, but instead, we will have a single builder object responsible for all the work, eliminating the need for intermediary objects.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.