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

feat: add static init for instances of Partition and Authentication #766

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

Conversation

aneojgurhem
Copy link
Contributor

@aneojgurhem aneojgurhem commented Oct 4, 2024

Motivation

  • Remove the need for the infrastructure to insert data in the database (authentication and partitions) as it needs to guess the database schema used in ArmoniK.Core.
  • Avoid perfoming init everytime an ArmoniK.Core container starts.

Description

An InitServices options class was introduced to initialize services. It contains two classes : Authentication and Partitionning to configure authentications and Partitions respectively. Authentication has several list of strings as fields: UserCertificates, Roles and Users. Those fields are JSON strings that are deserialized into corresponding objects that will be inserted into the database. Partitions works the same with the Partitionning class with the field Partitions.

These options can be configured as environment variables. Lists should be converted into an environment variable per element with its index as shown below.

See the classes Partition, User, Role and Certificate for the field that should be given has JSON strings.

export InitServices__Partitioning__Partitions__0='{"ParentPartitionIds":[],"PartitionId":"TestPartition0","PodConfiguration":null,"PodMax":100,"PodReserved":50,"PreemptionPercentage":20,"Priority":1}'
export InitServices__Authentication__Users__0='{"Name":"User1","Roles":["Role1"]}'
export InitServices__Authentication__Roles__0='{"Name":"Role1","Permissions":["Perm1"]}'
export InitServices__Authentication__UserCertificates__0='{"User": "User1", "CN": "CN1", "Fingerprint": "Fingerprint1"}'

Testing

Unit tests were added to validate the behavior of the implementation. ArmoniK.Core infrastructure was also updated to support a deployment with and without initialization activated in the compute plane and control plane. It can be toggled during deployment with the just option cinit.

Impact

  • Faster container startup.
  • Less requests in the database during container startup. It will improve performances by avoiding the same requests being performed by multiple instances during their startup.
  • Requests to MongoDB are entirely managed by ArmoniK.Core. External accesses are not needed anymore.
  • Breaks database scheme

Additional Information

Infrastructure will need to be modified to use the new way to initialize database. However, current (2.20 as of today) infrastructure should still be compatible with this development.

Integer are used as Id for Roles, Users and Certificates. When we will try to create new instances and insert them into the database through new APIs, we have two ways to generate the Id:

  • computing max + 1; the new Id may be duplicated in case of concurrency.
  • random value; less chances for the Id to already exists.
    In both cases, if the item can not be inserted into the database due to Id already existing, Id will have to be re-generated until we can insert the item successfully.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • I have thoroughly tested my modifications and added tests when necessary.
  • Tests pass locally and in the CI.
  • I have assessed the performance impact of my modifications.

.docs/content/1.concepts/3.authentication.md Outdated Show resolved Hide resolved
.docs/content/1.concepts/3.authentication.md Outdated Show resolved Hide resolved
.docs/content/1.concepts/3.authentication.md Outdated Show resolved Hide resolved
.docs/content/1.concepts/3.authentication.md Outdated Show resolved Hide resolved
@aneojgurhem aneojgurhem marked this pull request as draft October 10, 2024 05:43
@aneojgurhem aneojgurhem marked this pull request as ready for review October 11, 2024 12:45
dbrasseur-aneo
dbrasseur-aneo previously approved these changes Oct 11, 2024
lemaitre-aneo
lemaitre-aneo previously approved these changes Oct 18, 2024
Adaptors/MongoDB/src/Common/MongoCollectionProvider.cs Outdated Show resolved Hide resolved
Common/tests/Auth/AuthenticationIntegrationTest.cs Outdated Show resolved Hide resolved
@@ -362,8 +362,8 @@ public void GetIdentityFromIdShouldSucceed(int id,
ident.Username);
}

[TestCase("UserIdDontExist")]
public void GetIdentityFromIdShouldFail(string id)
[TestCase(1000)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[TestCase(1000)]
[TestCase(404)]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review in progress
Development

Successfully merging this pull request may close these issues.

4 participants