[doc][dash-p4] Update metering bucket design #605
Conversation
|
ok, looks like the SAI API doesn't have the stats API support for the table entries. I could not find any existing SAI API using this pattern. This change will need more time to change now.
|
|
After fixing the SAI/meta/style.pm to allow table entry stats, running into the following error. We may need to push a direct commit to OCP SAI to replace the existing experimental METER_BUCKET object with the METER_BUCKET_ENTRY and the corresponding APIs generated by this PR. |
|
This is because the entry stats functions are not generated in libsai. Feel free to take a look at the saiapi.cpp.j2. It should be straightforward to add (by copying the sai object one and do some small modifications) |
|
Better not do direct commit into SAI repo and diverge these 2 repos. |
@r12f, No its not about the libsai entry stats function. This is OCP SAI meta error complaining about the missing SAI API headers for the original Meter bucket object type which is not removed. |
I was thinking to create SAI OCP PR with the new Meter bucket table entry APIs exactly as it is generated here. |
|
Hi Mukesh, I see. The relationship between DASH SAI and OCP SAI is that - almost at any moment, the SAI API in DASH repo is always a superset of the OCP SAI. But once we updates the API directly there, everyone in DASH repo will produce the SAI without the new meter bucket API, unless they cherry pick the changes in this PR, hence the diverge. Since what we need to do is to pass the SAI checks, could we only submit the change that will make this PR pass to SAI, then bring the SAI back to DASH to unblock us? This is what we do for solving all previous SAI related issues too, such as spellchecks, SAI style checks and etc. |
Bring in SAI API changes from this DASH PR sonic-net/DASH#605 Signed-off-by: mukeshmv <[email protected]>
Bring in SAI API changes from this DASH PR sonic-net/DASH#605 Signed-off-by: mukeshmv <[email protected]>
@r12f , I still don't see the issue. Even if we directly commit to OCP SAI, DASH repo wont see the new SAI meter bucket entry APIs unless we update the SAI submodule in DASH, so there should be no diverge. We can update the SAI submodule as part of this PR so that the generated APIs are in sync with the SAI repo. I have created a OCP SAI PR, please take a look - |
|
Not easy to fix, not a backward compatible change. Update OCP SAI first, move SAI submodule to the latest. Per DASH Community call, review and accept, and push into OCP SAI. |
Bring in SAI API changes from this DASH PR sonic-net/DASH#605
|
@r12f the SAI PR has been merged. Can you update the SAI submodule to latest in this PR ? |
|
Hi Mukesh, ok. Do you mind to help with it? My internet access is horrible. Opening this page took me more than 10mins or so to retry... I don't feel I can get any real work done... |
|
I have commented the SAI PR as well. We will need get SAI merged back to DASH and get this PR merged. @mukeshmv The ideal way is to check in the style.pm file then update SAI in DASH to get the change unblocked. This keeps the DASH SAI being superset of OCP SAI and avoid the chances of unexpected change being merged into OCP SAI, as the generated API from main branch will going to revert this work. |
|
@r12f sure we will take care of this. As I explained before just fixing the style.pm was not sufficient to get this PR in as we were running into API missing errors. Since I don't have permission to add commit to this PR, I have created a duplicate PR with all your commits + updating to latest SAI. |
|
Perfect! Will try my best to open it and check it! Thanks Mukesh! (Do you still remember a mobile network called 3G? XD) |
|
ROFL 😊3G
From: Riff ***@***.***>
Sent: Wednesday, July 24, 2024 10:39 PM
To: sonic-net/DASH ***@***.***>
Cc: Kristina Moore ***@***.***>; Comment ***@***.***>
Subject: Re: [sonic-net/DASH] [doc][dash-p4] Update metering bucket design (PR #605)
Perfect! Will try my best to open it and check it! Thanks Mukesh! (Do you still remember a mobile network called 3G? XD)
—
Reply to this email directly, view it on GitHub<#605 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFJSI6CETQL3XO6UI23OUDLZOCFNNAVCNFSM6AAAAABLBNT5L2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBZGUYDCOBRGE>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
3G - Which corner of the world are you in :) |
) Bring in SAI API changes from this DASH PR sonic-net/DASH#605 Signed-off-by: siqbal1986 <[email protected]>
Problem
The current metering design uses SAI object to identify each metering bucket. This is not efficient from both CPU and memory perspective, because of multiple reasons:
What we are doing in this change
To solve these issues, we are changing the metering bucket from SAI object to SAI table entry. With this, we could add assumption that all metering buckets will be created along with the ENI without additional calls, which saves all the CPU and memory resources.