[VOL-4450] ONU upgrade to multiple ONU devices control insufficient, possibly leads to adapter crash
Signed-off-by: mpagenko <michael.pagenkopf@adtran.com>
Change-Id: If3a0ffefdaacbf7f0735c54bb3d352ef6b238630
diff --git a/VERSION b/VERSION
index 7ec1d6d..2f8bc15 100755
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.1.0
+2.1.1-dev247
diff --git a/internal/pkg/core/openonu.go b/internal/pkg/core/openonu.go
index 1afc7c7..7c5918a 100755
--- a/internal/pkg/core/openonu.go
+++ b/internal/pkg/core/openonu.go
@@ -511,7 +511,7 @@
if request != nil && len((*request).DeviceId) > 0 && (*request).Image.Version != "" {
loResponse := voltha.DeviceImageResponse{}
imageIdentifier := (*request).Image.Version
- downloadedToAdapter := false
+ downloadStartDone := false
firstDevice := true
var vendorID string
var onuVolthaDevice *voltha.Device
@@ -543,28 +543,26 @@
if firstDevice {
//start/verify download of the image to the adapter based on first found device only
// use the OnuVendor identification from first given device
+
+ // note: if the request was done for a list of devices on the Voltha interface, rwCore
+ // translates that into a new rpc for each device, hence each device will be the first device in parallel requests!
firstDevice = false
vendorID = onuVolthaDevice.VendorId
imageIdentifier = vendorID + imageIdentifier //head on vendor ID of the ONU
- logger.Debugw(ctx, "download request for file", log.Fields{"image-id": imageIdentifier})
+ logger.Infow(ctx, "download request for file",
+ log.Fields{"device-id": loDeviceID, "image-id": imageIdentifier})
- if !oo.pFileManager.ImageExists(ctx, imageIdentifier) {
- logger.Debugw(ctx, "start image download", log.Fields{"image-description": request})
- // Download_image is not supposed to be blocking, anyway let's call the DownloadManager still synchronously to detect 'fast' problems
- // the download itself is later done in background
- if err := oo.pFileManager.StartDownload(ctx, imageIdentifier, (*request).Image.Url); err == nil {
- downloadedToAdapter = true
- }
- //else: treat any error here as 'INVALID_URL' (even though it might as well be some issue on local FS, eg. 'INSUFFICIENT_SPACE')
- // otherwise a more sophisticated error evaluation is needed
- } else {
- // image already exists
- downloadedToAdapter = true
- logger.Debugw(ctx, "image already downloaded", log.Fields{"image-description": imageIdentifier})
+ // call the StartDownload synchronously to detect 'immediate' download problems
+ // the real download itself is later done in background
+ if fileState, err := oo.pFileManager.StartDownload(ctx, imageIdentifier, (*request).Image.Url); err == nil {
// note: If the image (with vendorId+name) has already been downloaded before from some other
- // valid URL, the current URL is just ignored. If the operators want to ensure that the new URL
+ // valid URL, the current download request is not executed (current code delivers URL error).
+ // If the operators want to ensure that the new URL
// is really used, then they first have to use the 'abort' API to remove the existing image!
// (abort API can be used also after some successful download to just remove the image from adapter)
+ if fileState == swupg.CFileStateDlSucceeded || fileState == swupg.CFileStateDlStarted {
+ downloadStartDone = true
+ } //else fileState may also indicate error situation, where the requested image is not ready to be used for other devices
}
} else {
//for all following devices verify the matching vendorID
@@ -574,9 +572,9 @@
vendorIDMatch = false
}
}
- if downloadedToAdapter && vendorIDMatch {
+ if downloadStartDone && vendorIDMatch {
// start the ONU download activity for each possible device
- logger.Debugw(ctx, "image download on omci requested", log.Fields{
+ logger.Infow(ctx, "request image download to ONU on omci ", log.Fields{
"image-id": imageIdentifier, "device-id": loDeviceID})
//onu upgrade handling called in background without immediate error evaluation here
// as the processing can be done for multiple ONU's and an error on one ONU should not stop processing for others
@@ -587,7 +585,9 @@
loDeviceImageState.ImageState.ImageState = voltha.ImageState_IMAGE_UNKNOWN
} else {
loDeviceImageState.ImageState.DownloadState = voltha.ImageState_DOWNLOAD_FAILED
- if !downloadedToAdapter {
+ if !downloadStartDone {
+ //based on above fileState more descriptive error codes would be possible, e.g
+ // IMAGE_EXISTS_WITH_DIFFERENT_URL - would require proto buf update
loDeviceImageState.ImageState.Reason = voltha.ImageState_INVALID_URL
} else { //only logical option is !vendorIDMatch
loDeviceImageState.ImageState.Reason = voltha.ImageState_VENDOR_DEVICE_MISMATCH
diff --git a/internal/pkg/swupg/file_download_manager.go b/internal/pkg/swupg/file_download_manager.go
index 900ba5a..37fa730 100755
--- a/internal/pkg/swupg/file_download_manager.go
+++ b/internal/pkg/swupg/file_download_manager.go
@@ -34,23 +34,29 @@
const cDefaultLocalDir = "/tmp" //this is the default local dir to download to
-type fileState uint32
+// FileState defines the download state of the ONU software image
+type FileState uint32
//nolint:varcheck, deadcode
const (
- cFileStateUnknown fileState = iota
- cFileStateDlStarted
- cFileStateDlSucceeded
- cFileStateDlFailed
- cFileStateDlAborted
- cFileStateDlInvalid
+ //CFileStateUnknown: the download state is not really known
+ CFileStateUnknown FileState = iota
+ //CFileStateDlStarted: the download to adapter has been started
+ CFileStateDlStarted
+ //CFileStateDlSucceeded: the download to adapter is successfully done (file exists and ready to use)
+ CFileStateDlSucceeded
+ //CFileStateDlFailed: the download to adapter has failed
+ CFileStateDlFailed
+ //CFileStateDlAborted: the download to adapter was aborted
+ CFileStateDlAborted
)
type downloadImageParams struct {
downloadImageName string
- downloadImageState fileState
+ downloadImageURL string
+ downloadImageState FileState
downloadImageLen int64
- downloadImageCrc uint32
+ downloadImageCRC uint32
downloadActive bool
downloadContextCancelFn context.CancelFunc
}
@@ -59,6 +65,7 @@
//FileDownloadManager structure holds information needed for downloading to and storing images within the adapter
type FileDownloadManager struct {
+ mutexFileState sync.RWMutex
mutexDownloadImageDsc sync.RWMutex
downloadImageDscSlice []downloadImageParams
dnldImgReadyWaiting map[string]requesterChannelMap
@@ -91,38 +98,46 @@
return dm.dlToAdapterTimeout
}
-//ImageExists returns true if the requested image already exists within the adapter
-func (dm *FileDownloadManager) ImageExists(ctx context.Context, aImageName string) bool {
- logger.Debugw(ctx, "checking on existence of the image", log.Fields{"image-name": aImageName})
- dm.mutexDownloadImageDsc.RLock()
- defer dm.mutexDownloadImageDsc.RUnlock()
-
- for _, dnldImgDsc := range dm.downloadImageDscSlice {
- if dnldImgDsc.downloadImageName == aImageName {
- //image found (by name)
- return true
- }
- }
- //image not found (by name)
- return false
-}
-
-//StartDownload returns true if the download of the requested image could be started for the given file name and URL
-func (dm *FileDownloadManager) StartDownload(ctx context.Context, aImageName string, aURLCommand string) error {
+//StartDownload returns FileState and error code from download request for the given file name and URL
+func (dm *FileDownloadManager) StartDownload(ctx context.Context, aImageName string, aURLCommand string) (FileState, error) {
logger.Infow(ctx, "image download-to-adapter requested", log.Fields{
"image-name": aImageName, "url-command": aURLCommand})
- loDownloadImageParams := downloadImageParams{
- downloadImageName: aImageName, downloadImageState: cFileStateDlStarted,
- downloadImageLen: 0, downloadImageCrc: 0}
- //try to download from http
- var err error
- if err = dm.downloadFile(ctx, aURLCommand, cDefaultLocalDir, aImageName); err == nil {
- dm.mutexDownloadImageDsc.Lock()
+
+ // keep a semaphore over the complete method in order to avoid parallel entrance to this method
+ // otherwise a temporary file state 'Started' could be indicated allowing start of ONU upgrade handling
+ // even though the download-start to adapter may fail (e.g on wrong URL) (delivering inconsistent download results)
+ // so once called the download-start of the first call must have been completely checked before another execution
+ // could still be limited to the same imageName, but that should be no real gain
+ dm.mutexFileState.Lock()
+ defer dm.mutexFileState.Unlock()
+ dm.mutexDownloadImageDsc.Lock()
+ var fileState FileState
+ var exists bool
+ if fileState, exists = dm.imageExists(ctx, aImageName, aURLCommand); !exists {
+ loDownloadImageParams := downloadImageParams{
+ downloadImageName: aImageName, downloadImageURL: aURLCommand, downloadImageState: CFileStateDlStarted,
+ downloadImageLen: 0, downloadImageCRC: 0}
dm.downloadImageDscSlice = append(dm.downloadImageDscSlice, loDownloadImageParams)
dm.mutexDownloadImageDsc.Unlock()
+ //start downloading from server
+ var err error
+ if err = dm.downloadFile(ctx, aURLCommand, cDefaultLocalDir, aImageName); err == nil {
+ //indicate download started correctly, complete download may run in background
+ return CFileStateDlStarted, nil
+ }
+ //return the error result of the download-request
+ return CFileStateUnknown, err
}
- //return the result of the start-request to comfort the core processing even though the complete download may go on in background
- return err
+ dm.mutexDownloadImageDsc.Unlock()
+ if fileState == CFileStateUnknown {
+ //cannot simply remove the existing file here - might still be used for running upgrades on different devices!
+ // (has to be removed by operator - cancel API)
+ logger.Errorw(ctx, "image download requested for existing file with different URL",
+ log.Fields{"image-description": aImageName, "url": aURLCommand})
+ return fileState, fmt.Errorf("existing file is based on different URL, requested URL: %s", aURLCommand)
+ }
+ logger.Debugw(ctx, "image download already started or done", log.Fields{"image-description": aImageName})
+ return fileState, nil
}
//GetImageBufferLen returns the length of the specified file in bytes (file size) - as detected after download
@@ -130,7 +145,7 @@
dm.mutexDownloadImageDsc.RLock()
defer dm.mutexDownloadImageDsc.RUnlock()
for _, dnldImgDsc := range dm.downloadImageDscSlice {
- if dnldImgDsc.downloadImageName == aFileName && dnldImgDsc.downloadImageState == cFileStateDlSucceeded {
+ if dnldImgDsc.downloadImageName == aFileName && dnldImgDsc.downloadImageState == CFileStateDlSucceeded {
//image found (by name) and fully downloaded
return dnldImgDsc.downloadImageLen, nil
}
@@ -139,6 +154,8 @@
}
//GetDownloadImageBuffer returns the content of the requested file as byte slice
+//precondition: it is assumed that a check is done immediately before if the file was downloaded to adapter correctly from caller
+// straightforward approach is here to e.g. immediately call and verify GetImageBufferLen() before this
func (dm *FileDownloadManager) GetDownloadImageBuffer(ctx context.Context, aFileName string) ([]byte, error) {
file, err := os.Open(filepath.Clean(cDefaultLocalDir + "/" + aFileName))
if err != nil {
@@ -221,6 +238,24 @@
// FileDownloadManager private (unexported) methods -- start
+//imageExists returns current ImageState and if the requested image already exists within the adapter
+//precondition: at calling this method mutexDownloadImageDsc must already be at least RLocked by the caller
+func (dm *FileDownloadManager) imageExists(ctx context.Context, aImageName string, aURL string) (FileState, bool) {
+ logger.Debugw(ctx, "checking on existence of the image", log.Fields{"image-name": aImageName})
+ for _, dnldImgDsc := range dm.downloadImageDscSlice {
+ if dnldImgDsc.downloadImageName == aImageName {
+ //image found (by name)
+ if dnldImgDsc.downloadImageURL == aURL {
+ //image found (by name and URL)
+ return dnldImgDsc.downloadImageState, true
+ }
+ return CFileStateUnknown, true //use fileState to indicate URL mismatch for existing file
+ }
+ }
+ //image not found (by name)
+ return CFileStateUnknown, false
+}
+
//imageLocallyDownloaded returns true if the requested image already exists within the adapter
// requires mutexDownloadImageDsc to be locked (at least RLocked)
func (dm *FileDownloadManager) imageLocallyDownloaded(ctx context.Context, aImageName string) bool {
@@ -228,7 +263,7 @@
for _, dnldImgDsc := range dm.downloadImageDscSlice {
if dnldImgDsc.downloadImageName == aImageName {
//image found (by name)
- if dnldImgDsc.downloadImageState == cFileStateDlSucceeded {
+ if dnldImgDsc.downloadImageState == CFileStateDlSucceeded {
logger.Debugw(ctx, "image has been fully downloaded", log.Fields{"image-name": aImageName})
return true
}
@@ -266,7 +301,7 @@
if dnldImgDsc.downloadImageName == aImageName {
//image found (by name) - need to write changes on the original map
dm.downloadImageDscSlice[imgKey].downloadActive = false
- dm.downloadImageDscSlice[imgKey].downloadImageState = cFileStateDlSucceeded
+ dm.downloadImageDscSlice[imgKey].downloadImageState = CFileStateDlSucceeded
dm.downloadImageDscSlice[imgKey].downloadImageLen = aFileSize
logger.Debugw(ctx, "imageState download succeeded", log.Fields{
"image-name": aImageName, "image-size": aFileSize})
@@ -294,6 +329,7 @@
urlBase, err1 := url.Parse(aURLCommand)
if err1 != nil {
logger.Errorw(ctx, "could not set base url command", log.Fields{"url": aURLCommand, "error": err1})
+ dm.removeImage(ctx, aFileName, false) //wo FileSystem access
return fmt.Errorf("could not set base url command: %s, error: %s", aURLCommand, err1)
}
urlParams := url.Values{}
@@ -303,6 +339,7 @@
reqExist, errExist2 := http.NewRequest("HEAD", urlBase.String(), nil)
if errExist2 != nil {
logger.Errorw(ctx, "could not generate http head request", log.Fields{"url": urlBase.String(), "error": errExist2})
+ dm.removeImage(ctx, aFileName, false) //wo FileSystem access
return fmt.Errorf("could not generate http head request: %s, error: %s", aURLCommand, errExist2)
}
ctxExist, cancelExist := context.WithDeadline(ctx, time.Now().Add(3*time.Second)) //waiting for some fast answer
@@ -313,6 +350,7 @@
if respExist == nil {
logger.Errorw(ctx, "http head from url error - no status, aborting", log.Fields{"url": urlBase.String(),
"error": errExist3})
+ dm.removeImage(ctx, aFileName, false) //wo FileSystem access
return fmt.Errorf("http head from url error - no status, aborting: %s, error: %s",
aURLCommand, errExist3)
}
@@ -322,6 +360,7 @@
if respExist.StatusCode != http.StatusMethodNotAllowed {
logger.Errorw(ctx, "http head from url: file does not exist here, aborting", log.Fields{"url": urlBase.String(),
"error": errExist3, "status": respExist.StatusCode})
+ dm.removeImage(ctx, aFileName, false) //wo FileSystem access
return fmt.Errorf("http head from url: file does not exist here, aborting: %s, error: %s, status: %d",
aURLCommand, errExist3, respExist.StatusCode)
}
@@ -403,7 +442,7 @@
//removeImage deletes the given image according to the Image name from filesystem and downloadImageDscSlice
func (dm *FileDownloadManager) removeImage(ctx context.Context, aImageName string, aDelFs bool) {
- logger.Debugw(ctx, "remove the image from Adapter", log.Fields{"image-name": aImageName})
+ logger.Debugw(ctx, "remove the image from Adapter", log.Fields{"image-name": aImageName, "del-fs": aDelFs})
dm.mutexDownloadImageDsc.RLock()
defer dm.mutexDownloadImageDsc.RUnlock()
diff --git a/internal/pkg/swupg/omci_onu_upgrade.go b/internal/pkg/swupg/omci_onu_upgrade.go
index 140c6f9..fe4aac2 100755
--- a/internal/pkg/swupg/omci_onu_upgrade.go
+++ b/internal/pkg/swupg/omci_onu_upgrade.go
@@ -626,7 +626,7 @@
} else {
fileLen, err = oFsm.pDownloadManager.getImageBufferLen(ctx, oFsm.pImageDsc.Name, oFsm.pImageDsc.LocalDir)
}
- if err != nil || fileLen > int64(cMaxUint32) {
+ if err != nil || fileLen == 0 || fileLen > int64(cMaxUint32) {
oFsm.volthaDownloadReason = voltha.ImageState_UNKNOWN_ERROR //something like 'LOCAL_FILE_ERROR' would be better (proto)
oFsm.mutexUpgradeParams.Unlock()
logger.Errorw(ctx, "OnuUpgradeFsm abort: problems getting image buffer length", log.Fields{
@@ -640,11 +640,11 @@
}
//copy file content to buffer
- oFsm.imageBuffer = make([]byte, fileLen)
+ var imageBuffer []byte
if oFsm.useAPIVersion43 {
- oFsm.imageBuffer, err = oFsm.pFileManager.GetDownloadImageBuffer(ctx, oFsm.imageIdentifier)
+ imageBuffer, err = oFsm.pFileManager.GetDownloadImageBuffer(ctx, oFsm.imageIdentifier)
} else {
- oFsm.imageBuffer, err = oFsm.pDownloadManager.getDownloadImageBuffer(ctx, oFsm.pImageDsc.Name, oFsm.pImageDsc.LocalDir)
+ imageBuffer, err = oFsm.pDownloadManager.getDownloadImageBuffer(ctx, oFsm.pImageDsc.Name, oFsm.pImageDsc.LocalDir)
}
if err != nil {
oFsm.volthaDownloadReason = voltha.ImageState_UNKNOWN_ERROR //something like 'LOCAL_FILE_ERROR' would be better (proto)
@@ -658,12 +658,17 @@
}(pBaseFsm)
return
}
+ //provide slice capacity already with the reserve of one section to avoid inflation of the slice to double size at append
+ oFsm.imageBuffer = make([]byte, fileLen, fileLen+cOmciDownloadSectionSize)
+ //better use a copy of the read image buffer in case the buffer/file is modified from outside,
+ // this also limits the slice len to the expected maximum fileLen
+ copy(oFsm.imageBuffer, imageBuffer)
oFsm.noOfSections = uint32(fileLen / cOmciDownloadSectionSize)
if fileLen%cOmciDownloadSectionSize > 0 {
bufferPadding := make([]byte, cOmciDownloadSectionSize-uint32((fileLen)%cOmciDownloadSectionSize))
//expand the imageBuffer to exactly fit multiples of cOmciDownloadSectionSize with padding
- oFsm.imageBuffer = append(oFsm.imageBuffer[:(fileLen)], bufferPadding...)
+ oFsm.imageBuffer = append(oFsm.imageBuffer, bufferPadding...)
oFsm.noOfSections++
}
oFsm.origImageLength = uint32(fileLen)