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

https://issues.redhat.com/browse/ACM-17497 - Add Architecture section to Edge Manager #7459

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

amolnar-rh
Copy link
Collaborator

@amolnar-rh amolnar-rh commented Feb 5, 2025

Issue: https://issues.redhat.com/browse/ACM-17497

To preview, select the file in this folder with the same name as the branch name from this PR.

Comment on lines 21 to 22
Field Selector:: Filters and selectors for {rhem} objects based on the values of specific resource fields.
Field selectors follow the same syntax, principles, and support the same operators as Kubernetes field and label selectors.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we need to get into this here, but we actually support a superset of Kubernetes.

@amolnar-rh amolnar-rh force-pushed the FC-arch branch 5 times, most recently from 9f3179b to 08969f6 Compare February 11, 2025 14:16
Copy link
Contributor

@avishayt avishayt left a comment

Choose a reason for hiding this comment

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

Looks better! Some improvements in the comments. Do we want to mention that we have two databases (Postgres and Redis), or is it enough to just say "database" to keep it generic and not overload with details?

Copy link
Contributor

@swopebe swopebe left a comment

Choose a reason for hiding this comment

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

Starting to look really good. Some comments, discussion points, changes needed in the comments.

The API server is a core component of the {rhem} service that exposes both agent-facing and user-facing API endpoints.
The {rhem} service communicates with various external systems to authenticate and authorize users, get mTLS certificates signed, or query configuration for managed devices.

For the external communication, the {rhem} service's API server exposes two endpoints:

Choose a reason for hiding this comment

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

Redundant with the first sentence ("[...] exposes both agent-facing and user-facing API endpoints"). Also, saying "For the external communication" right after "communicates with various external systems" seems to imply the API endpoints are used with those external systems.

The user- and agent-facing APIs are for the users and agents to connect to the RHEM service, respectively. The RHEM service, in turn, connects to the external systems for configuration, authentication, authorization, and having certificates signed.

@amolnar-rh amolnar-rh force-pushed the FC-arch branch 3 times, most recently from 7e2261f to 6dd34b5 Compare February 17, 2025 13:44
@amolnar-rh amolnar-rh force-pushed the FC-arch branch 2 times, most recently from 283ed7a to 1102f99 Compare February 17, 2025 13:53
@amolnar-rh amolnar-rh requested a review from swopebe February 17, 2025 14:03
Copy link
Contributor

@avishayt avishayt left a comment

Choose a reason for hiding this comment

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

Some minor edits, take them or leave them. Otherwise LGTM.


* enroll devices into the service
* periodically check with the service for changes in the device specification, such as changes to the operating system, configuration, and applications
* apply any updates independently from the service

Choose a reason for hiding this comment

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

Not sure If we also want to mention as an agent task:
"report a systemd unit status as an application, if it was defined to be monitored on service side"

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fzdarsky @avishayt What do you think about including it?^

Copy link
Contributor

Choose a reason for hiding this comment

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

Good comment. I would include it more generally:
"report the status of the device and its applications"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you! Added it under agent tasks

@amolnar-rh amolnar-rh force-pushed the FC-arch branch 2 times, most recently from f2cd79a to e1f7dbb Compare February 20, 2025 15:54
Copy link

openshift-ci bot commented Feb 24, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amolnar-rh, fzdarsky, swopebe

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@amolnar-rh amolnar-rh merged commit c82fb4e into 2.13_stage Feb 24, 2025
0 of 2 checks passed
@amolnar-rh amolnar-rh deleted the FC-arch branch February 24, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants