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: aliyun datasource support crawl metadata at once #5942

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

jinkkkang
Copy link
Contributor

@jinkkkang jinkkkang commented Dec 25, 2024

feat: aliyun datasource support crawl metadata at once

In the pr: https://github.com/canonical/cloud-init/pull/5838,
we discussed the optimization of improving the startup
speed of cloudinit, which is to provide a path in the IMDS server
to return all instance metadata information at once. This PR
is mainly due to the support of this feature in the aliyun data
source. We separated the data source from EC2, of course, we also
reused some tool functions of EC2. The benefits of the separation
are obvious. Prior to this, some behaviors of EC2 affected Alibaba
Cloud, such as the device number attribute in network configuration.
https://bugs.launchpad.net/cloud-init/+bug/1917875

The update made this time are as follows
1. Separate the Alibaba Cloud data source from ec2 and make it
   independent
2. Alibaba Cloud data sources will prioritize obtaining metadata
   information from the new path at once. If it fails, it will return
   to the previous directory tree format
3. The network_config in aliyun data sources has streamlined
   the logic in EC2, using network card names to sort routing
   priorities
4. aliyun data sources support vendor data

Additional Context

Test Steps

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@jinkkkang
Copy link
Contributor Author

jinkkkang commented Dec 25, 2024

cloudinit log in alibaba cloud instance:
aliyun-cloud-init-log.txt

cloudinit log in aws cloud instance, aws ec2 data source has removed content related to Aliyun, which has no impact on EC2
aws-cloud-init-after-delete-aliyun-log.txt

@jinkkkang jinkkkang force-pushed the aliyun_data_source_update branch 2 times, most recently from 2ec5ea7 to 0a731d9 Compare December 25, 2024 07:46
@jinkkkang
Copy link
Contributor Author

@holmanb @TheRealFalcon this update is a follow-up to the previous PR discussion. Please help review it

@holmanb holmanb self-assigned this Jan 7, 2025
@jinkkkang
Copy link
Contributor Author

@holmanb hello, could you please take a look.

Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 31, 2025
@github-actions github-actions bot closed this Feb 7, 2025
@jinkkkang
Copy link
Contributor Author

@holmanb @TheRealFalcon There are quite a few modifications. If there are any parts that need to be updated, please provide suggestions. We would be grateful and look forward to your feedback

@TheRealFalcon TheRealFalcon reopened this Feb 7, 2025
@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Feb 7, 2025
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @jinkkkang! The general approach looks good. I've added some requests to the PR below.

Using the meta-data/all endpoint should certainly help with performance. The logs appear to be using this endpoint. Is this endpoint currently available on Alibaba cloud?

@jinkkkang
Copy link
Contributor Author

Thanks for this PR @jinkkkang! The general approach looks good. I've added some requests to the PR below.

Using the meta-data/all endpoint should certainly help with performance. The logs appear to be using this endpoint. Is this endpoint currently available on Alibaba cloud?

Thank you for your reply, We have provided support for meta-data/all endpoint in all regions on Alibaba cloud, it is available now @holmanb

@jinkkkang jinkkkang force-pushed the aliyun_data_source_update branch 4 times, most recently from d40765a to aa1da70 Compare February 11, 2025 06:00
@jinkkkang
Copy link
Contributor Author

@holmanb Thank you for your reply and fix mypy error code. all seems to be okay now

@ani-sinha
Copy link
Contributor

Commenting in order to get notified when this merges.

@jinkkkang jinkkkang force-pushed the aliyun_data_source_update branch from aa1da70 to e2b0700 Compare February 19, 2025 02:00
@jinkkkang jinkkkang force-pushed the aliyun_data_source_update branch from e2b0700 to d0eeea8 Compare February 19, 2025 02:26
@jinkkkang
Copy link
Contributor Author

@holmanb Sorry, I missed some mypy errors earlier and have fixed them all. Please help confirm again that there are no issues and assist with merging

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for working on this @jinkkkang!


@return: The API token or None if unavailable.
"""
# if self.cloud_name not in IDMSV2_SUPPORTED_CLOUD_PLATFORMS:
Copy link
Member

Choose a reason for hiding this comment

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

There is no reason to keep this commented code.

If _api_token is unset on AWS, attempt to refresh the token via a PUT
and then return the updated token header.
"""
# if self.cloud_name not in IDMSV2_SUPPORTED_CLOUD_PLATFORMS:
Copy link
Member

Choose a reason for hiding this comment

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

There is no reason to keep this commented code.

@holmanb holmanb merged commit 27adc8e into canonical:main Feb 20, 2025
22 checks passed
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.

4 participants