Skip to content

[docker-ptf]: Choose PTF image based on branch#14362

Closed
opcoder0 wants to merge 5 commits intosonic-net:masterfrom
opcoder0:feat/ptf-py3-tests-verification
Closed

[docker-ptf]: Choose PTF image based on branch#14362
opcoder0 wants to merge 5 commits intosonic-net:masterfrom
opcoder0:feat/ptf-py3-tests-verification

Conversation

@opcoder0
Copy link
Contributor

@opcoder0 opcoder0 commented Sep 2, 2024

Description of PR

This PR adds condition to pipeline to choose PTF image tag based on the branch. All master branch test scripts have been migrated to Python 3 and can run in py3only PTF image. For all feature branches branched out of master the PTF image tag can be py3only. For all other branches we continue to the internal tag with mixed Py2+Py3 image.

Summary:
Fixes # Not applicable

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

All PTF scripts have been migrated to Python 3. And the PTF image with Python 3 only has been built and published. This PR now modifies the PR testing pipeline to choose the PTF image based on the branch. For master branch py3only tag can be chosen. For all other release branches, we use the existing mixed image.

How did you do it?

Check the "MGMT_BRANCH" to detect current branch and pass the ptf_imagetag to the pytest framework via common extra parameters.

How did you verify/test it?

Can only be verified here.

Any platform specific information?

None

Supported testbed topology if it's a new test case?

None

Documentation

Not applicable.

* Add condition to pipeline to choose PTF image tag
  based on the branch. All master branch test scripts hve been
  migrated to Python 3 and can run in "py3only" PTF image. For
  all other branches we continue to use "internal" a mixed Py2+Py3
  image.
parent_commit=$(git rev-parse HEAD^1)
master_commit=$(git rev-parse master)
branch_from_master=0
if [ "$parent_commit" == "$master_commit" ]
Copy link
Contributor

@xwjiang-ms xwjiang-ms Sep 2, 2024

Choose a reason for hiding this comment

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

Compare the commit id doesn't make sense to me. Once a branch like 202411 branched out from master in the future, and certainly it could use py3only image, but compare commit id would cause branch_from_master=0 and use internal image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. Thanks. I will probably use a list of known branches that use mixed and the rest can use py3only.

PTF_IMAGETAG="--ptf_imagetag=internal"
fi
common_extra_params="${{ parameters.COMMON_EXTRA_PARAMS }} $PTF_IMAGETAG"
echo "##vso[task.setvariable variable=COMMON_EXTRA_PARAMS;]$common_extra_params"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is to create pipeline variable "COMMON_EXTRA_PARAMS". Then subsequent pipeline tasks can access to this pipeline variable.

Since the common extra params is only used this task, it is unnecessary to create such pipeline variable. Just use bash variable should be enough. Then, in line 223, refer to bash variable instead of pipeline parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I have modified that bit.

… the script instead of modifying pipeline var
Comment on lines +208 to +221
ptf_image_type="py3only"
for old_branch in ${{ parameters.MIXED_PTF_IMAGE_BRANCHES }}
do
if [ $old_branch == "${{ parameters.MGMT_BRANCH }}" ]
then
ptf_image_type="mixed"
break
fi
done
PTF_IMAGETAG="--ptf_imagetag=py3only"
if [ $ptf_image_type == "mixed" ]
then
PTF_IMAGETAG="--ptf_imagetag=internal"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

ptf_image_type is only used to determine whether should modify PTF_IMAGETAG, how about not use this extra variable,but just modify PTF_IMAGETAG in the for-loop statement when matching if condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've modified it.

@opcoder0 opcoder0 marked this pull request as draft September 2, 2024 06:24
@opcoder0 opcoder0 requested a review from lerry-lee September 2, 2024 06:28
@opcoder0 opcoder0 marked this pull request as ready for review September 3, 2024 05:40
@opcoder0
Copy link
Contributor Author

opcoder0 commented Sep 3, 2024

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 14362 in repo sonic-net/sonic-mgmt

@wangxin
Copy link
Collaborator

wangxin commented Sep 3, 2024

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@wangxin
Copy link
Collaborator

wangxin commented Sep 3, 2024

After offline discussion, the better way to specify ptf_imagetag is to update the default value for ptf_imagetag variable in ansible playbook for each branch:
https://github.com/sonic-net/sonic-mgmt/blob/master/ansible/roles/vm_set/tasks/add_topo.yml#L10
https://github.com/sonic-net/sonic-mgmt/blob/master/ansible/roles/vm_set/tasks/renumber_topo.yml#L15

@opcoder0
Copy link
Contributor Author

opcoder0 commented Sep 4, 2024

After offline discussion, the better way to specify ptf_imagetag is to update the default value for ptf_imagetag variable in ansible playbook for each branch: https://github.com/sonic-net/sonic-mgmt/blob/master/ansible/roles/vm_set/tasks/add_topo.yml#L10 https://github.com/sonic-net/sonic-mgmt/blob/master/ansible/roles/vm_set/tasks/renumber_topo.yml#L15

Thank you @wangxin. I will be closing this PR as the sequence of work to be done changes.

@opcoder0 opcoder0 closed this Sep 4, 2024
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