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

Control interface shall be created only for configured for it workloads #322

Closed
2 of 4 tasks
krucod3 opened this issue Jul 19, 2024 · 3 comments
Closed
2 of 4 tasks
Assignees
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Milestone

Comments

@krucod3
Copy link
Contributor

krucod3 commented Jul 19, 2024

Description

The basic idea here is to skip the creation of the control interface if the configuration of the workload does not contain a control interface field. Doing this will spare some green threads on Ankaios side and the creation of fifo pipes that will never be used, improving the overall performance and stability.

Goals

  • A Control Interface instance shall only be created for configured workloads.

Final result

Summary

To be filled when the final solution is sketched.

Tasks

  • Rename the PipesChannelContext to something meaningful, like ControlInterface
    You can even split this into its own PR and reduce the chances of conflicts with main
  • Refactor a bit the creation of the Control Interface and unify the concepts there: Remove the extra functions for creating one;
  • ...
  • Add an stest that checks that a workload that did not have a ControlInterface access and is updated to have a ControlInterface access is restarted.
@krucod3 krucod3 added the enhancement New feature or request. Issue will appear in the change log "Features" label Jul 19, 2024
@krucod3 krucod3 added this to the v0.5 milestone Jul 25, 2024
@GabyUnalaq
Copy link
Contributor

Currently working on this.

@inf17101
Copy link
Contributor

inf17101 commented Aug 14, 2024

The PR for the refactoring is #345.

Summary of decisions for the refactoring:

@krucod3: The ControlInterfaceInfo must be mocked because it is needed in the RuntimeManager where it is constructed within methods. This new call must be intercepted then.

With the new control interface module structure together with mockall implementing a ControlInterface::from_info method calling the constructor with the parameters from the Info struct is not testable because a mixture of a mock and not mock inside the same module is not testable. The same is valid for a try_from implementation, but anyway a try_from is signaling a type conversion, but we are creating fifo files from an info struct (metadata) which is not type conversion and introduces side effects for the reader.

We decided to refactor to a single method to create an ControlInterface: its constructor ControlInterface::new(...) which is already designed for that. The ControlInterfaceInfo is used as a mock inside the RuntimeManager and also needed there as a mock because its constructor is called within bodies of functions. In general that ControlInterfaceInfo is mocked is making the restructuring more compilcated since it enforces to use also the MockAuthorizer which is one of its members.

With the new structure of the control_interface module there where lots of problems with imports of mocks and public API exports at the beginning. Especially, inside the new control_interface.rs module file (public api exports and imports are not allowed inside the same file as the compiler treats it as duplicated imports an throws errors). To ensure that the mock and the production structs are found and treated correctly and not have issues with the duplicated imports we decided to not have an public API export for Authorizer and ControlInterfaceInfo. An alternative would be to put the control_interface.rs implementation into another sub module called "control_interface_impl", then we can use public API exports and correct imports for mocks and production code within the impl, but this leads to unnecessary modules with naming sugar.

At the end, achieved is a common and single interface of creating the control interface in different places where also logging can be handled differently in each client and a new module structure with renamed structures that are now more related to the ControlInterface. For the import paths and public API exports we had to make some compromise.

@krucod3
Copy link
Contributor Author

krucod3 commented Sep 19, 2024

All done here.

@krucod3 krucod3 closed this as completed Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Projects
None yet
Development

No branches or pull requests

3 participants