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

Adding subscription support for ethernet interfaces. #140

Merged
merged 5 commits into from
May 31, 2024

Conversation

nagarwal03
Copy link
Contributor

@nagarwal03 nagarwal03 commented May 21, 2024

@ranjinidn
Copy link
Contributor

Ping @sachinholla @anand-kumar-subramanian @mbalachandar for review.

if inParams.oper == SUBSCRIBE {
var _intfTypeList []E_InterfaceType

if idx == "*" || idx != "0" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be if idx != "*" && idx != "0" {/* error */}. Current condition rejects the wildcard key and hence blocks traget_defined subscription at a higher level like interface list

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 for reviewing the PR @sachinholla. I have addressed the comments. Please review.

if strings.HasPrefix(targetUriPath, "/openconfig-interfaces:interfaces/interface/config") {
tblList = append(tblList, "PORT")
} else if strings.HasPrefix(targetUriPath, "/openconfig-interfaces:interfaces/interface/state") {
tblList = append(tblList, "PORT_TABLE")
Copy link
Contributor

Choose a reason for hiding this comment

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

What about /interfaces/interface/state/counters path? It should have been mapped to COUNTERS, right?

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 Subscribe_intf_get_counters_xfmr handles the mapping of interfaces/interface/state/counters to COUNTERS_PORT_NAME_MAP

@@ -725,7 +752,9 @@ var DbToYang_intf_eth_port_config_xfmr SubTreeXfmrDbToYang = func(inParams XfmrP
intfsObj := getIntfsRoot(inParams.ygRoot)
pathInfo := NewPathInfo(inParams.uri)
uriIfName := pathInfo.Var("name")
ifName := uriIfName
ifName := *(&uriIfName)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do? How it is different from normal assignment??

ifName := uriIfName
ifName := *(&uriIfName)

log.Infof("DbToYang_intf_eth_port_config_xfmr: Interface name : %s ", ifName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a verbose log

var result XfmrSubscOutParams

if inParams.subscProc == TRANSLATE_SUBSCRIBE {
log.Info("Subscribe_intf_eth_port_config_xfmr: inParams.subscProc: ", inParams.subscProc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this log is unnecessary.. You have already checked for the inParams.subscProc value and taken a decision. Make it a verbose log if you want to retain


if ifName == "" {
ifName = "*"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shorthand: ifName := pathInfo.StringVar("name", "*")

result.dbDataMap = RedisDbSubscribeMap{db.ConfigDB: {
"PORT": {ifName: {"autoneg": "auto-negotiate", "speed": "port-speed"}}}}

log.Info("Subscribe_intf_eth_port_config_xfmr: result ", result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to verbose log please

}

var DbToYangPath_intf_eth_port_config_path_xfmr PathXfmrDbToYangFunc = func(params XfmrDbToYgPathParams) error {
log.Info("DbToYangPath_intf_eth_port_config_path_xfmr: params: ", params)
Copy link
Contributor

Choose a reason for hiding this comment

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

User verbose log while logging the whole struct..

}

if (params.tblName == "PORT") && (len(params.tblKeyComp) > 0) {
params.ygPathKeys[intfRoot+"/name"] = *(&params.tblKeyComp[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why *(&xyz) do?

ifKey = "*"
} else {
sonicIfName := &uriIfName
ifKey = *sonicIfName
Copy link
Contributor

Choose a reason for hiding this comment

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

Why take an address and then dereference? I think just the following is sufficient -- ifKey := pathInfo.StringVar("name", "*")

@nagarwal03 nagarwal03 force-pushed the Add-oc-intf-eth-subs branch 3 times, most recently from b3ab98b to eb56bf4 Compare May 30, 2024 09:02
Copy link
Contributor

@sachinholla sachinholla left a comment

Choose a reason for hiding this comment

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

Changes look fine.. Please verify target_defined subscription for /interfaces/interface[name=*] path as well.

@kwangsuk kwangsuk merged commit 81bf799 into sonic-net:master May 31, 2024
2 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.

5 participants