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

Initial PTP documentation for ATIP #542

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Conversation

mchiappero
Copy link
Contributor

No description provided.

@mchiappero mchiappero marked this pull request as draft January 17, 2025 13:28
@mchiappero
Copy link
Contributor Author

The CAPI section is still WIP; I will also update the commit message soon. But you can have a look at the wording in the meantime. Thanks!


Replace `p1p1` with name of the interface to be used for PTP.

The following sections will provide guidance on how to install and configure PTP on SUSE Edge specifically, but familiarity with basic PTP concepts is expected. For a brief overview of PTP and the implementation included in ATIP, refer to https://documentation.suse.com/sles/15-SP5/html/SLES-all/cha-tuning-ptp.html[].
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably refer to the latest 15-SP6 SLES docs here

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use https://documentation.suse.com/sles/html/SLES-all/cha-tuning-ptp.html --it will always redirect to the latest version.

The `linuxptp` package included in ATIP does not enable `ptp4l` and `phc2sys` by default. If having them enabled at provisioning time is your preference, list them in the `systemd` section, as in the above example. This can be helpful if their system specific configuration files are also deployed at provisioning time through the directed network provisioning flow (see <<ptp-capi>>).
====

You can now follow the usual process to build the image as per the https://github.com/suse-edge/edge-image-builder/blob/release-1.0/docs/building-images.md[EIB Documentation] and use it to deploy your cluster. If you are new to EIB start from xref:../components/edge-image-builder.adoc#components-eib[] instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again I think we should prefer the product docs where possible, we have coverage of EIB usage in the ATIP docs and also the EIB quickstart we could link to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have never noticed eib was in the quickstart section, I have linked from the components/upstream for this reason. Should we drop the last link, leave it as is or change it to the top of the quickstart guide?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the most relevant link is the ATIP section on building the image e.g https://documentation.suse.com/suse-edge/3.1/html/edge/atip-automated-provisioning.html#id-image-creation-2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be inclined to refer to the EIB one, the ATIP is a duplicate (which must be updated accordingly BTW)... but either way is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, for internal links (pointing to some section of the Edge doc itself), please do not use http links or xrefs. Rather use the ID of the target section for linking:

If you are new to EIB start from <<components-eib>> instead.

or, if you want to add a custom link text:

If you are new to EIB start with the <<components-eib,Edge Image Builder Introduction>> instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused here, I have found some documentation stating the opposite. What is the reason for preferring one over the other?

Copy link
Contributor

@hardys hardys left a comment

Choose a reason for hiding this comment

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

Overall looks great - a couple of small comments added

@mchiappero mchiappero force-pushed the ptp-doc branch 3 times, most recently from 71000c8 to 348cc01 Compare January 17, 2025 17:17
@mchiappero mchiappero marked this pull request as ready for review January 17, 2025 17:18
@mchiappero
Copy link
Contributor Author

It requires further work, there are other possibilities that have not been covered, but I think this could be it for now. If it looks good enough to you, I'd merge and do more updates later.

hardys
hardys previously approved these changes Jan 18, 2025
atanasdinov
atanasdinov previously approved these changes Jan 18, 2025
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

LGTM! Only some ATIP references have to be adjusted.

asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
nodeName: "localhost.localdomain"
----

Besides other variables, the above definition must be completed with the interface name and with the other Cluster API objects, as described in xref:atip-automated-provisioning.adoc#single-node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this link going to work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch - no it doesn't - perhaps we can just replace it with <<atip-automated-provisioning>> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The filename was wrong, but it doesn't work still. I am unable to understand why the others do but this one doesn't. Will likely switch to this format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't make it work. It's not a problem with the macro itself, there were two identical IDs in the referenced file, but it doesn't work anyway. I'd push and come back to it later.

Copy link
Contributor

@hardys hardys Jan 20, 2025

Choose a reason for hiding this comment

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

@mchiappero I tested and the format I mentioned works, it just doesn't link to the specific section - can we just go with that for now?

@mchiappero mchiappero dismissed stale reviews from atanasdinov and hardys via 493f066 January 20, 2025 09:53
Copy link
Collaborator

@dariavladykina dariavladykina left a comment

Choose a reason for hiding this comment

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

Hi! Please see some linguistic suggestions here. Thanks!

asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/quickstart/eib.adoc Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
Add a couple of anchors to enable references from other files.

Signed-off-by: Marco Chiappero <[email protected]>
Copy link
Collaborator

@fsundermeyer fsundermeyer left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nitpicks from my side

asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved

Both daemons are required for the system synchronization to fully work and must be correctly configured according to your setup, as covered in <<ptp-telco-config>>.

The easiest and best way to integrate PTP in your downstream cluster is to add the `linuxptp` package under `packageList` in the Edge Image Builder (EIB) definition file. This way the PTP control plane software will be installed automatically during the cluster provisioning. See the xref:../quickstart/eib.adoc#eib-configuring-rpm-packages[EIB documentation] for more information on installing packages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The easiest and best way to integrate PTP in your downstream cluster is to add the `linuxptp` package under `packageList` in the Edge Image Builder (EIB) definition file. This way the PTP control plane software will be installed automatically during the cluster provisioning. See the xref:../quickstart/eib.adoc#eib-configuring-rpm-packages[EIB documentation] for more information on installing packages.
The easiest and best way to integrate PTP in your downstream cluster is to add the `linuxptp` package under `packageList` in the Edge Image Builder (EIB) definition file. This way the PTP control plane software will be installed automatically during the cluster provisioning. See the <<eib-configuring-rpm-packages,EIB documentation>> for more information on installing packages.

asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
asciidoc/product/atip-features.adoc Outdated Show resolved Hide resolved
hardys
hardys previously approved these changes Jan 20, 2025
Copy link
Contributor

@hardys hardys left a comment

Choose a reason for hiding this comment

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

lgtm but if possible I'd like to fix the link before we merge

Introduce an overview of how to install and configure PTP in ATIP with
telco specific PTP profiles. This documentation is expected to be
expanded over time, but this commit provides guidance on:

- software installation
- software configuration
- mentions to PTP and "directed network provisioning"

Acked-by: Daria Vladykina <[email protected]>
Acked-by: Frank Sundermeyer <[email protected]>
Signed-off-by: Marco Chiappero <[email protected]>
Add PTP to the least of features and to the tech-preview list.

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero mchiappero merged commit 2c12ec8 into suse-edge:main Jan 20, 2025
1 check passed
@hardys
Copy link
Contributor

hardys commented Jan 20, 2025

Thanks @mchiappero I'd suggest we merge this now so we can cut the release branch, then iterate on any remaining changes via follow-up PRs

@mchiappero
Copy link
Contributor Author

By the way, thank you all for your input!

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.

5 participants