From dcd4a43af1e8f46d52e5f2b772cb368991ecea98 Mon Sep 17 00:00:00 2001 From: Joseph Richard Date: Mon, 26 May 2025 15:55:30 -0600 Subject: [PATCH 1/2] Allow duplicate keys in ptpconfig When initially designing PTP config parsing, there was an assumption that for each section, all of the keys were unique. Based on this assumption, key:value pairs for a given section were stored as a map[string][string]. With the introduction of unicast_master_table, this assumption is no longer true, as in the case of the UDPv4 key, there can be multiple entries with the same key. As such, the config data structure is changed to maintain a list of individual key:value pairs, rather than a map of keys to values. In addition, some restructuring was done for places that relied on the pre-existing structure, moving most of that functionality to utility functions in config.go --- pkg/daemon/config.go | 195 ++++++++++++++++++++++++++++++------------ pkg/daemon/daemon.go | 32 ++----- pkg/daemon/metrics.go | 14 +-- 3 files changed, 151 insertions(+), 90 deletions(-) diff --git a/pkg/daemon/config.go b/pkg/daemon/config.go index fd007830..9caa44de 100644 --- a/pkg/daemon/config.go +++ b/pkg/daemon/config.go @@ -1,6 +1,7 @@ package daemon import ( + "bytes" "encoding/json" "errors" "fmt" @@ -19,6 +20,13 @@ import ( ptpv1 "github.com/openshift/ptp-operator/api/v1" ) +// predefined config section names +const ( + GlobalSectionName = "[global]" + NmeaSectionName = "[nmea]" + UnicastSectionName = "[unicast_master_table]" +) + // LinuxPTPUpdate controls whether to update linuxPTP conf // and contains linuxPTP conf to be updated. It's rendered // and passed to linuxptp instance by daemon. @@ -29,19 +37,77 @@ type LinuxPTPConfUpdate struct { defaultPTP4lConfig []byte } +type ptp4lConfOption struct { + key string + value string +} + type ptp4lConfSection struct { sectionName string - options map[string]string + options []ptp4lConfOption } -type ptp4lConf struct { +// Ptp4lConf is a structure to represent a parsed ptpconfig, +// which can then be rendered to a string again. +type Ptp4lConf struct { sections []ptp4lConfSection - mapping []string profile_name string clock_type event.ClockType gnss_serial_port string // gnss serial port } +func (conf *Ptp4lConf) getPtp4lConfOptionOrEmptyString(sectionName string, key string) (string, bool) { + for _, section := range conf.sections { + if section.sectionName == sectionName { + for _, option := range section.options { + if option.key == key { + return option.value, true + } + } + } + } + + return "", false +} + +func (conf *Ptp4lConf) setPtp4lConfOption(sectionName string, key string, value string, overwrite bool) { + var updatedSection ptp4lConfSection + index := -1 + for i, section := range conf.sections { + if section.sectionName == sectionName { + updatedSection = section + index = i + } + } + if index < 0 { + newSectionOptions := make([]ptp4lConfOption, 0) + updatedSection = ptp4lConfSection{options: newSectionOptions, sectionName: sectionName} + index = len(conf.sections) + conf.sections = append(conf.sections, updatedSection) + } + + //Stop now if initializing section without option + if key == "" { + return + } + found := false + if overwrite { + for i := range updatedSection.options { + if updatedSection.options[i].key == key { + updatedSection.options[i] = ptp4lConfOption{key: key, value: value} + found = true + } + } + } + // Append unless already overwrote it. + if !found { + updatedSection.options = append(updatedSection.options, ptp4lConfOption{key: key, value: value}) + } + + //Update section in conf + conf.sections[index] = updatedSection +} + func NewLinuxPTPConfUpdate() (*LinuxPTPConfUpdate, error) { if _, err := os.Stat(PTP4L_CONF_FILE_PATH); err != nil { if os.IsNotExist(err) { @@ -60,7 +126,7 @@ func NewLinuxPTPConfUpdate() (*LinuxPTPConfUpdate, error) { } func (l *LinuxPTPConfUpdate) UpdateConfig(nodeProfilesJson []byte) error { - if string(l.appliedNodeProfileJson) == string(nodeProfilesJson) { + if bytes.Equal(l.appliedNodeProfileJson, nodeProfilesJson) { return nil } if nodeProfiles, ok := tryToLoadConfig(nodeProfilesJson); ok { @@ -113,42 +179,37 @@ func tryToLoadOldConfig(nodeProfilesJson []byte) ([]ptpv1.PtpProfile, bool) { return []ptpv1.PtpProfile{*ptpConfig}, true } -// Takes as input a PtpProfile.Ptp4lConf and outputs as ptp4lConf struct -func (output *ptp4lConf) populatePtp4lConf(config *string) error { +// PopulatePtp4lConf takes as input a PtpProfile.Ptp4lConf string and outputs as ptp4lConf struct +func (conf *Ptp4lConf) PopulatePtp4lConf(config *string) error { var currentSectionName string - var currentSection ptp4lConfSection - output.sections = make([]ptp4lConfSection, 0) - globalIsDefined := false + conf.sections = make([]ptp4lConfSection, 0) hasSlaveConfigDefined := false - + ifaceCount := 0 if config != nil { for _, line := range strings.Split(*config, "\n") { line = strings.TrimSpace(line) if strings.HasPrefix(line, "#") { continue } else if strings.HasPrefix(line, "[") { - if currentSectionName != "" { - output.sections = append(output.sections, currentSection) - } currentLine := strings.Split(line, "]") - if len(currentLine) < 2 { return errors.New("Section missing closing ']': " + line) } - currentSectionName = fmt.Sprintf("%s]", currentLine[0]) - if currentSectionName == "[global]" { - globalIsDefined = true + if currentSectionName != GlobalSectionName && currentSectionName != NmeaSectionName && currentSectionName != UnicastSectionName { + ifaceCount++ } - currentSection = ptp4lConfSection{options: map[string]string{}, sectionName: currentSectionName} + conf.setPtp4lConfOption(currentSectionName, "", "", false) } else if currentSectionName != "" { split := strings.IndexByte(line, ' ') if split > 0 { - currentSection.options[line[:split]] = line[split:] - if (line[:split] == "masterOnly" && line[split:] == "0") || - (line[:split] == "serverOnly" && line[split:] == "0") || - (line[:split] == "slaveOnly" && line[split:] == "1") || - (line[:split] == "clientOnly" && line[split:] == "1") { + key := line[:split] + value := strings.TrimSpace(line[split:]) + conf.setPtp4lConfOption(currentSectionName, key, value, false) + if (key == "masterOnly" && value == "0") || + (key == "serverOnly" && value == "0") || + (key == "slaveOnly" && value == "1") || + (key == "clientOnly" && value == "1") { hasSlaveConfigDefined = true } } @@ -156,28 +217,44 @@ func (output *ptp4lConf) populatePtp4lConf(config *string) error { return errors.New("Config option not in section: " + line) } } - if currentSectionName != "" { - output.sections = append(output.sections, currentSection) - } - } - - if !globalIsDefined { - output.sections = append(output.sections, ptp4lConfSection{options: map[string]string{}, sectionName: "[global]"}) } if !hasSlaveConfigDefined { // No Slave Interfaces defined - output.clock_type = event.GM - } else if len(output.sections) > 2 { + conf.clock_type = event.GM + } else if ifaceCount > 1 { // Multiple interfaces with at least one slave Interface defined - output.clock_type = event.BC + conf.clock_type = event.BC } else { // Single slave Interface defined - output.clock_type = event.OC + conf.clock_type = event.OC } + return nil } +// ExtendGlobalSection extends Ptp4lConf struct with fields not from ptp4lConf +func (conf *Ptp4lConf) ExtendGlobalSection(profileName string, messageTag string, socketPath string, pProcess string) { + conf.profile_name = profileName + conf.setPtp4lConfOption(GlobalSectionName, "message_tag", messageTag, true) + if socketPath != "" { + conf.setPtp4lConfOption(GlobalSectionName, "uds_address", socketPath, true) + } + if gnssSerialPort, ok := conf.getPtp4lConfOptionOrEmptyString(GlobalSectionName, "ts2phc.nmea_serialport"); ok { + conf.gnss_serial_port = strings.TrimSpace(gnssSerialPort) + conf.setPtp4lConfOption(GlobalSectionName, "ts2phc.nmea_serialport", GPSPIPE_SERIALPORT, true) + } + if _, ok := conf.getPtp4lConfOptionOrEmptyString(GlobalSectionName, "leapfile"); ok || pProcess == ts2phcProcessName { // not required to check process if leapfile is always included + conf.setPtp4lConfOption(GlobalSectionName, "leapfile", fmt.Sprintf("%s/%s", config.DefaultLeapConfigPath, os.Getenv("NODE_NAME")), true) + } +} + +// AddInterfaceSection adds interface to Ptp4lConf +func (conf *Ptp4lConf) AddInterfaceSection(iface string) { + ifaceSectionName := fmt.Sprintf("[%s]", iface) + conf.setPtp4lConfOption(ifaceSectionName, "", "", false) +} + func getSource(isTs2phcMaster string) event.EventSource { if ts2phcMaster, err := strconv.ParseBool(strings.TrimSpace(isTs2phcMaster)); err == nil { if ts2phcMaster { @@ -195,7 +272,7 @@ func getSource(isTs2phcMaster string) event.EventSource { // (UNTIL next device section). // 2. Port section - any other section not starting with < (e.g. [eth0]) is the port section. // Multiple port sections are allowed. Each port participates in SyncE communication. -func (conf *ptp4lConf) extractSynceRelations() *synce.Relations { +func (conf *Ptp4lConf) extractSynceRelations() *synce.Relations { var err error r := &synce.Relations{ Devices: []*synce.Config{}, @@ -207,7 +284,8 @@ func (conf *ptp4lConf) extractSynceRelations() *synce.Relations { var extendedTlv, networkOption int = synce.ExtendedTLV_DISABLED, synce.SYNCE_NETWORK_OPT_1 for _, section := range conf.sections { - if strings.HasPrefix(section.sectionName, "[<") { + sectionName := section.sectionName + if strings.HasPrefix(sectionName, "[<") { if synceRelationInfo.Name != "" { if len(ifaces) > 0 { synceRelationInfo.Ifaces = ifaces @@ -226,23 +304,23 @@ func (conf *ptp4lConf) extractSynceRelations() *synce.Relations { } extendedTlv, networkOption = synce.ExtendedTLV_DISABLED, synce.SYNCE_NETWORK_OPT_1 - synceRelationInfo.Name = re.ReplaceAllString(section.sectionName, "") - if networkOptionStr, ok := section.options["network_option"]; ok { + synceRelationInfo.Name = re.ReplaceAllString(sectionName, "") + if networkOptionStr, ok := conf.getPtp4lConfOptionOrEmptyString(sectionName, "network_option"); ok { if networkOption, err = strconv.Atoi(strings.TrimSpace(networkOptionStr)); err != nil { glog.Errorf("error parsing `network_option`, setting network_option to default 1 : %s", err) } } - if extendedTlvStr, ok := section.options["extended_tlv"]; ok { + if extendedTlvStr, ok := conf.getPtp4lConfOptionOrEmptyString(sectionName, "extended_tlv"); ok { if extendedTlv, err = strconv.Atoi(strings.TrimSpace(extendedTlvStr)); err != nil { glog.Errorf("error parsing `extended_tlv`, setting extended_tlv to default 1 : %s", err) } } synceRelationInfo.NetworkOption = networkOption synceRelationInfo.ExtendedTlv = extendedTlv - } else if strings.HasPrefix(section.sectionName, "[{") { - synceRelationInfo.ExternalSource = re.ReplaceAllString(section.sectionName, "") - } else if strings.HasPrefix(section.sectionName, "[") && section.sectionName != "[global]" { - iface := re.ReplaceAllString(section.sectionName, "") + } else if strings.HasPrefix(sectionName, "[{") { + synceRelationInfo.ExternalSource = re.ReplaceAllString(sectionName, "") + } else if strings.HasPrefix(sectionName, "[") && sectionName != GlobalSectionName { + iface := re.ReplaceAllString(sectionName, "") ifaces = append(ifaces, iface) } } @@ -255,52 +333,55 @@ func (conf *ptp4lConf) extractSynceRelations() *synce.Relations { return r } -func (conf *ptp4lConf) renderSyncE4lConf(ptpSettings map[string]string) (configOut string, relations *synce.Relations) { +// RenderSyncE4lConf outputs synce4l config as string +func (conf *Ptp4lConf) RenderSyncE4lConf(ptpSettings map[string]string) (configOut string, relations *synce.Relations) { configOut = fmt.Sprintf("#profile: %s\n", conf.profile_name) relations = conf.extractSynceRelations() relations.AddClockIds(ptpSettings) deviceIdx := 0 + for _, section := range conf.sections { configOut = fmt.Sprintf("%s\n%s", configOut, section.sectionName) if strings.HasPrefix(section.sectionName, "[<") { - if _, found := section.options["clock_id"]; !found { - section.options["clock_id"] = relations.Devices[deviceIdx].ClockId + if _, found := conf.getPtp4lConfOptionOrEmptyString(section.sectionName, "clock_id"); !found { + conf.setPtp4lConfOption(section.sectionName, "clock_id", relations.Devices[deviceIdx].ClockId, true) deviceIdx++ } } - for k, v := range section.options { + for _, option := range section.options { + k := option.key + v := option.value configOut = fmt.Sprintf("%s\n%s %s", configOut, k, v) } } return } -func (conf *ptp4lConf) renderPtp4lConf() (configOut string, ifaces config.IFaces) { +// RenderPtp4lConf outputs ptp4l config as string +func (conf *Ptp4lConf) RenderPtp4lConf() (configOut string, ifaces config.IFaces) { configOut = fmt.Sprintf("#profile: %s\n", conf.profile_name) - conf.mapping = nil var nmea_source event.EventSource for _, section := range conf.sections { configOut = fmt.Sprintf("%s\n%s", configOut, section.sectionName) - if section.sectionName == "[nmea]" { - if source, ok := section.options["ts2phc.master"]; ok { + if section.sectionName == NmeaSectionName { + if source, ok := conf.getPtp4lConfOptionOrEmptyString(section.sectionName, "ts2phc.master"); ok { nmea_source = getSource(source) } } - if section.sectionName != "[global]" && section.sectionName != "[nmea]" { + if section.sectionName != GlobalSectionName && section.sectionName != NmeaSectionName && section.sectionName != UnicastSectionName { i := section.sectionName i = strings.ReplaceAll(i, "[", "") i = strings.ReplaceAll(i, "]", "") - conf.mapping = append(conf.mapping, i) iface := config.Iface{Name: i} - if source, ok := section.options["ts2phc.master"]; ok { + if source, ok := conf.getPtp4lConfOptionOrEmptyString(section.sectionName, "ts2phc.master"); ok { iface.Source = getSource(source) } else { // if not defined here, use source defined at nmea section iface.Source = nmea_source } - if masterOnly, ok := section.options["masterOnly"]; ok { + if masterOnly, ok := conf.getPtp4lConfOptionOrEmptyString(section.sectionName, "masterOnly"); ok { // TODO add error handling iface.IsMaster, _ = strconv.ParseBool(strings.TrimSpace(masterOnly)) } @@ -310,7 +391,9 @@ func (conf *ptp4lConf) renderPtp4lConf() (configOut string, ifaces config.IFaces PhcId: iface.PhcId, }) } - for k, v := range section.options { + for _, option := range section.options { + k := option.key + v := option.value configOut = fmt.Sprintf("%s\n%s %s", configOut, k, v) } } diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index cd918ff6..498e3c51 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -475,41 +475,25 @@ func (dn *Daemon) applyNodePtpProfile(runID int, nodeProfile *ptpv1.PtpProfile) continue } - output := &ptp4lConf{} - err = output.populatePtp4lConf(configInput) + output := &Ptp4lConf{} + err = output.PopulatePtp4lConf(configInput) if err != nil { printNodeProfile(nodeProfile) return err } clockType := output.clock_type - output.profile_name = *nodeProfile.Name if nodeProfile.Interface != nil && *nodeProfile.Interface != "" { - output.sections = append([]ptp4lConfSection{{ - options: map[string]string{}, - sectionName: fmt.Sprintf("[%s]", *nodeProfile.Interface)}}, output.sections...) + output.AddInterfaceSection(*nodeProfile.Interface) } else { iface := string("") nodeProfile.Interface = &iface } - for index, section := range output.sections { - if section.sectionName == "[global]" { - section.options["message_tag"] = messageTag - if socketPath != "" { - section.options["uds_address"] = socketPath - } - if gnssSerialPort, ok := section.options["ts2phc.nmea_serialport"]; ok { - output.gnss_serial_port = strings.TrimSpace(gnssSerialPort) - section.options["ts2phc.nmea_serialport"] = GPSPIPE_SERIALPORT - } - if _, ok := section.options["leapfile"]; ok || pProcess == ts2phcProcessName { // not required to check process if leapfile is always included - section.options["leapfile"] = fmt.Sprintf("%s/%s", config.DefaultLeapConfigPath, os.Getenv("NODE_NAME")) - } - output.sections[index] = section - } - } + output.ExtendGlobalSection(*nodeProfile.Name, messageTag, socketPath, pProcess) + + //output, messageTag, socketPath, GPSPIPE_SERIALPORT, update_leapfile, os.Getenv("NODE_NAME") // This adds the flags needed for monitor addFlagsForMonitor(p, configOpts, output, dn.stdoutToSocket) @@ -517,9 +501,9 @@ func (dn *Daemon) applyNodePtpProfile(runID int, nodeProfile *ptpv1.PtpProfile) var relations *synce.Relations var ifaces config.IFaces if pProcess == syncEProcessName { - configOutput, relations = output.renderSyncE4lConf(nodeProfile.PtpSettings) + configOutput, relations = output.RenderSyncE4lConf(nodeProfile.PtpSettings) } else { - configOutput, ifaces = output.renderPtp4lConf() + configOutput, ifaces = output.RenderPtp4lConf() for i := range ifaces { ifaces[i].PhcId = ptpnetwork.GetPhcId(ifaces[i].Name) } diff --git a/pkg/daemon/metrics.go b/pkg/daemon/metrics.go index 3870d635..c16fcf55 100644 --- a/pkg/daemon/metrics.go +++ b/pkg/daemon/metrics.go @@ -667,7 +667,7 @@ func extractPTP4lEventState(output string) (portId int, role ptpPortRole) { return } -func addFlagsForMonitor(process string, configOpts *string, conf *ptp4lConf, stdoutToSocket bool) { +func addFlagsForMonitor(process string, configOpts *string, conf *Ptp4lConf, stdoutToSocket bool) { switch process { case "ptp4l": // If output doesn't exist we add it for the prometheus exporter @@ -678,15 +678,9 @@ func addFlagsForMonitor(process string, configOpts *string, conf *ptp4lConf, std } if !strings.Contains(*configOpts, "--summary_interval") { - for index, section := range conf.sections { - if section.sectionName == "[global]" { - _, exist := section.options["summary_interval"] - if !exist { - glog.Info("adding summary_interval 1 to print summary messages to stdout for ptp4l to use prometheus exporter") - section.options["summary_interval"] = "1" - } - conf.sections[index] = section - } + _, exist := conf.getPtp4lConfOptionOrEmptyString(GlobalSectionName, "summary_interval") + if !exist { + conf.setPtp4lConfOption(GlobalSectionName, "summary_interval", "1", true) } } } From 84a96cd621aa514c9f95137df9de463798e8e232 Mon Sep 17 00:00:00 2001 From: Joseph Richard Date: Wed, 18 Jun 2025 10:32:42 -0600 Subject: [PATCH 2/2] masterOnly 0 under [global] section shouldn't affect clockType --- pkg/daemon/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/daemon/config.go b/pkg/daemon/config.go index 9caa44de..ed11cd6a 100644 --- a/pkg/daemon/config.go +++ b/pkg/daemon/config.go @@ -206,7 +206,7 @@ func (conf *Ptp4lConf) PopulatePtp4lConf(config *string) error { key := line[:split] value := strings.TrimSpace(line[split:]) conf.setPtp4lConfOption(currentSectionName, key, value, false) - if (key == "masterOnly" && value == "0") || + if (key == "masterOnly" && value == "0" && currentSectionName != GlobalSectionName) || (key == "serverOnly" && value == "0") || (key == "slaveOnly" && value == "1") || (key == "clientOnly" && value == "1") {